Testing unsupported values in enums

Engineering
1

Hi,
this is an issue that @seanm brought to the table in this gerrit topic:
http://review.source.kitware.com/#/c/23321/

So the question is that some compilers throw a warning for some values that are not certainly listed among the allowed values. However, in some ITK classes they are present and hence are tested for the sake of completeness and code coverage, even if being a marginal case. When the case is included in the implementation file, it is usually expected to throw an exception.

There are several possibilities brought by @seanm in the mentioned topic:

Since ITK has adopted C++11 and the transition is mature before the release of ITK 5.0.0, we have a good chance to act on this.

Suggestions and constructive criticism on either choice is welcome.

Thanks.

2

Interesting issue, @jhlegarreta Looks like the minimal fix to the issue brought up by @seanm would be to go for the “Fixed Unscoped Enumerations” approach, from https://wiki.sei.cmu.edu/confluence/display/cplusplus/INT50-CPP.+Do+not+cast+to+an+out-of-range+enumeration+value As follows:

enum FrustumRotationPlaneType : int
{
RotateInXZPlane = 1,
RotateInYZPlane
};

However, a scoped enum certainly seems appropriate in this particular case. If so, I think the names of the enum constants could be shortened, for example:

enum class FrustumRotationPlaneType
{
XZ = 1,
YZ
};

To be used as FrustumRotationPlaneType::XZ and FrustumRotationPlaneType::YZ.

By the way, when a function deals with each and every named enum constant of an enumerated type separately, it seems like a good idea to use a switch, instead of having multiple “if” statements. The compiler may give a warning when the switch has accidentally forgotten to handle some of the enum constants.

My 2 cents, Niels

1 Like
3

I think it is fairly common to map something to the ITK enums. It may be mapping a selection from a pull down GUI to the ITK enum. Another example in SimpleITK, we redefine the enums, and them map them to the ITK enums. We have tests to verify the mapping. Also from just the SWIG interface, used in wrapping, there is another mapping from ints to enums.

The point is that the case of handling out of bounds is useful in ITK code because it is common to force-ably map ints to the enums. When an out of bounds case is encountered, it make the most since to throw an exception and let the application handle the error. Calling abort for an erroneous value will terminate the program. When interactively using ITK via Python or any other language, this may be fairly easy todo.

We need to define the desired behavior, and implement out code to meet that requirement. We should not let the C++ language dictate our behavior.

4

To be clear, it’s not just that ‘some compilers throw a warning’, it’s that it is not valid C++ to convert any old int into into a plain old enum. ITK currently does that in 1 or 2 places, ex:

https://open.cdash.org/testDetails.php?test=636229345&build=5380430

ITK/Modules/Filtering/LabelMap/test/itkMergeLabelMapFilterTest1.cxx:82:22: runtime error: load of value 10, which is not a valid value for type ‘itk::MergeLabelMapFilter<itk::LabelMap<itk::LabelObject<unsigned char, 2> > >::MethodChoice’

Indeed, with “Fixed Unscoped Enumerations” it is valid C++. That could be a quite backwards-compatible change for ITK to adopt, i.e. just stick " : int" after most enum definitions. That won’t change their size and would fix this particular issue. Or, leave the enums as they are, and just eliminate the 1 or 2 places where out of range values are forced in.

Cheers,

Sean

1 Like
5

I do agree that this is a serious bug, that you have found, @seanm Note however, that static_cast<FrustumRotationPlaneType>(i) is OK (valid C++, no undefined behavior) for any integer i between 0 and 3 (inclusive). You see, FrustumRotationPlaneType is defined as follows:

typedef enum {
  RotateInXZPlane = 1,
  RotateInYZPlane
} FrustumRotationPlaneType;

So it supports integer values 1 and 2, as well as the bitwise OR of these values. So that includes 3. And zero is also included, of course.

static_cast<FrustumRotationPlaneType>(i) does produce undefined behavior when i >= 4, as you already found out (for i == 4).

My 2 cents, Niels