clang -Wunreachable-code-break and -Wself-assign-field

Engineering
1

Hi all,

Two warnings on my bots I’m not sure what you all prefer to do about:

  1. warning: ‘break’ will never be executed [-Wunreachable-code-break]
    case itk::ImageIOBase::DOUBLE:
      std::cerr << "Hashing is not supporting for float and double images." << std::endl;
      itkGenericExceptionMacro( "Hashing is not supported for images of float or doubles." );
      break; // <- warning here
    case itk::ImageIOBase::UNKNOWNCOMPONENTTYPE:
    default:
      assert( false ); // should never get here unless we forgot a type
      itkGenericExceptionMacro( "Logic error!" );

Do you prefer we remove the break, or I disable this warning flag? It’s the only such warning in all of ITK currently, but I concede it’s maybe not the most useful warning ever.

  1. warning: assigning field to itself [-Wself-assign-field]
/** Concept requiring T to have operator =.  (BOOST) */
template< typename T >
struct Assignable {
  struct Constraints {
    void constraints()
    {
      a = a; // <- warning here
      const_constraints(a);
    }

I’ve often seen “a = a” as a way to silence unused variable warnings, but this doesn’t look like that…

Cheers,

Sean

1 Like
2

Remove both offenders in a proposed patch?

3

I think just removing the break is a good idea.

For the constraint, I think it does some meta-programing type stuff and should not be removed.

4

I meant remove statement a=a;, not the constraint.

5

So the a=a statement isn’t exercising the existence of the operator= ? The comment above says “Concept requiring T to have operator =”, but I don’t really grok the code.

6

The code is not run. But the address of the function is taken so is must compile. I believe the intent is to ensure the class has operator=.

Now modern has std::is_assignable which does a similar and has appropriate variants:

Many of these constraints could be replaced with the modern type_traits, and static_asserts, but that is a different project.

1 Like