Project

General

Profile

Support #694

In test files: when is assert.H needed?

Added by John Abbott almost 9 years ago. Updated almost 7 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Tidying
Start date:
08 May 2015
Due date:
% Done:

100%

Estimated time:
3.51 h
Spent time:

Description

Some test files explicitly include CoCoA/assert.H but many do not.

Check that it is truly needed when it is present.


Related issues

Related to CoCoALib - Design #642: Move code in test file into namespace CoCoAClosed2014-10-29

Related to CoCoALib - Support #695: Remove cruft from test filesNew2015-05-08

History

#1 Updated by John Abbott almost 9 years ago

Actually I am rather surprised that it is not necessary for all tests.

Investigate why it works without explicitly including assert.H

#2 Updated by John Abbott about 8 years ago

  • Target version changed from CoCoALib-0.99540 Feb 2016 to CoCoALib-0.99550 spring 2017

#3 Updated by John Abbott over 7 years ago

  • Status changed from New to In Progress
  • Assignee set to John Abbott
  • % Done changed from 0 to 50

The explanation is fairly obvious (and simple): some header files already include assert.H. Here is the current list:

DenseUPolyClean.H:#include "CoCoA/assert.H" 
DistrMPolyClean.H:#include "CoCoA/assert.H" 
DistrMPolyInlFpPP.H:#include "CoCoA/assert.H" 
DistrMPolyInlPP.H:#include "CoCoA/assert.H" 
DynamicBitset.H:#include "CoCoA/assert.H" 
FloatApprox.H:#include "CoCoA/assert.H" 
MachineInt.H:#include "CoCoA/assert.H" 
OrdvArith-OpenMP.H:#include "CoCoA/assert.H" 
OrdvArith.H:#include "CoCoA/assert.H" 
SmallFpDoubleImpl.H:#include "CoCoA/assert.H" 
SmallFpImpl.H:#include "CoCoA/assert.H" 
SmallFpLogImpl.H:#include "CoCoA/assert.H" 
TmpPPVector.H:#include "CoCoA/assert.H" 
ring.H:#include "CoCoA/assert.H" 

The only remaining doubt is whether the tests should have an #include "assert.H" directive even when it is not strictly needed. If a header file which currently include assert.H is changed not to include it then some tests may need to be changed to include assert.H explicitly...

What to do?

#4 Updated by John Abbott over 7 years ago

  • % Done changed from 50 to 60

Anna points out that if we do this for assert.H then why shouldn't we do it for all header files... at which point the situation becomes unmanageable. Also if someone modifies the library (incl. changing some header files) then it is to be expected that code depending on the library may well need to be modified (e.g. change the list of explicitly/directly included files).

The original question was about whether some #include "assert.H" directives were superfluous. Really it does not matter if they are superfluous, and the time spent "cleaning and tidying" superfluous includes is probably better spent elsewhere.

#5 Updated by John Abbott over 7 years ago

  • % Done changed from 60 to 70

Since all tests must include GlobalManager.H and that file indirectly includes assert.H, there is no need for any test to specifically include assert.H.

Anna suggested moving the definition of the TEST_ASSERT macro to assert.H to avoid having separate definitions in each test file. This is an appealing idea, but we need a new name beginning with CoCoA.

#6 Updated by John Abbott over 7 years ago

JAA suggests the short name CoCoA_CHECK, but Anna thinks that is not so clear.
Anna suggests the longer name CoCoA_ASSERT_ALWAYS; she argues that the length is not so important as it is used really only in tests.

#7 Updated by Anna Maria Bigatti over 7 years ago

John Abbott wrote:

JAA suggests the short name CoCoA_CHECK, but Anna thinks that is not so clear.
Anna suggests the longer name CoCoA_ASSERT_ALWAYS; she argues that the length is not so important as it is used really only in tests.

sligthly shorter CoCoA_ASSERT_TEST makes it clear that the functions do something very similar, and that this is used (only?) in the tests.

#8 Updated by John Abbott over 7 years ago

  • Status changed from In Progress to Feedback
  • % Done changed from 70 to 90

In the end I chose CoCoA_ASSERT_ALWAYS as it is pretty clear what it means.

I have removed every include directive for assert.H (even the commented out ones).

Everything seems to work OK, so will check in (shortly).

I'll leave the status as "feedback" for a few weeks.

#9 Updated by Anna Maria Bigatti over 7 years ago

Works fine for me (clang)

#10 Updated by John Abbott over 7 years ago

  • Status changed from Feedback to Closed
  • % Done changed from 90 to 100

#11 Updated by Anna Maria Bigatti almost 7 years ago

  • Estimated time set to 3.51 h

Also available in: Atom PDF