Clang related fix

[Alpar Juttner]

Current clang version complains about the following function definition in concepts::Readmap.

      Value operator[](const Key &) const {
        return *(static_cast<Value *>(0)+1);
      }

The attached patch fixes it by changing to

        return Value();

But this means that ReadMap::Value must be default constructible. (Thus tests/map_tests.cc had to be changed accordingly.) Unfortunately I couldn't come up with any better solution.

Any idea?

[Balazs Dezso]

Maybe something like this:

Value operator[](const Key &) const {
  char mem[sizeof(Value)];
  for (std::size_t i = 0; i < sizeof(Value); ++i) {
    mem[i] = char(0);
  }
  return *(reinterpret_cast<Value *>(mem));
}

[Alpar Juttner]

It seems that

return *(reinterpret_cast<Value *>(sizeof(Value)));

also works, and it may even be safer.

[Peter Kovacs]

Replying to Alpar Juttner:

It seems that

return *(reinterpret_cast<Value *>(sizeof(Value)));

also works, and it may even be safer.

I don't understand the usage of sizeof here. Why is your suggestion better than

return *(reinterpret_cast<Value *>(0));

? Anyway, I even don't understand the +1 in the current code.

In fact, in other methods in maps.h, these two lines are used instead:

Value *r = 0;
return *r;

It seems to be equivalent to the line I suggested above. (Is there any difference?) I would prefer using the same solution for each similar method.

[Alpar Juttner]

Replying to Peter Kovacs:

I don't understand the usage of sizeof here. Why is your suggestion better than

return *(reinterpret_cast<Value *>(0));

? Anyway, I even don't understand the +1 in the current code.

The reason is that compilers do not like referencing to NULL. Instead of an arbitrary nonzero value, sizeof(Value) --- or equivalently +1 --- is used to ensure that the value of the underlying pointer is properly aligned. That's why I think this is safer than the solution proposed by Balazs.

In fact, in other methods in maps.h, these two lines are used instead:

Value *r = 0;
return *r;

It seems to be equivalent to the line I suggested above. (Is there any difference?) I would prefer using the same solution for each similar method.

I believe, if you let the CLANG people know this, they will soon fix their compiler to fail on this too. So, yes, we should do the same here as above.

[Alpar Juttner]

For the record, here is what the compilers say about the alternatives.

code	GCC 9 debug	GCC 9 optimized	CLANG 9 debug	CLANG 9 optimized
return *(static_cast<Value *>(0));	OK	OK	warning(1)	warning(1)
return *(static_cast<Value *>(0)+1);	OK	OK	warning(2)	warning(2)
return *(static_cast<Value *>(sizeof(Value)));	error(3)	error(3)	error(4)	error(4)
return *(reinterpret_cast<Value *>(sizeof(0)));	OK	OK	warning(1)	warning(1)
return *(reinterpret_cast<Value *>(0)+1);	OK	OK	warning(2)	warning(2)
return *(reinterpret_cast<Value *>(sizeof(Value)));	OK	OK	OK	OK

(1)

warning: indirection of non-volatile null pointer will be deleted, not trap [-Wnull-dereference]
        return *(static_cast<Value *>(0));
               ^~~~~~~~~~~~~~~~~~~~~~~~~~

(2)

warning: performing pointer arithmetic on a null pointer has undefined behavior if the offset is nonzero [-Wnull-pointer-arithmetic]
        return *(static_cast<Value *>(0)+1);
                 ~~~~~~~~~~~~~~~~~~~~~~~^

(3)

error: invalid static_cast from type ‘long unsigned int’ to type ‘lemon::concepts::ReadMap<lemon::concepts::Digraph::Arc, int>::Value*’ {aka ‘int*’}
   54 |         return *(static_cast<Value *>(sizeof(Value)));
      |                 ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

(4)

error: cannot cast from type 'unsigned long' to pointer type 'lemon::concepts::ReadMap<lemon::concepts::Digraph::Arc, int>::Value *' (aka 'int *')
        return *(static_cast<Value *>(sizeof(Value)));
                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[Peter Kovacs]

I see, thank you for the info.

However, I still don't like this solution too much:

return *(reinterpret_cast<Value *>(sizeof(Value)));

because sizeof(Value) is only used for getting a non-zero arbitrary integer value. Couldn't we use a simple constant instead of it, e.g. 42? Or would it trigger another warning?

Among the alternatives listed so far, I still prefer the one we use in the other methods, although it is 2 lines, not only 1.

Anyway, these methods in concept classes need not be run, only compiled. So what about throwing an exception instead of such pointer tricks?

[Alpar Juttner]

However, I still don't like this solution too much [...] because sizeof(Value) is only used for getting a non-zero arbitrary integer value. Couldn't we use a simple constant instead of it, e.g. 42? Or would it trigger another warning?

As I explained, sizeof(Value) is not just an arbitrary value. It is used because is ensures the proper alignment of the underlying pointer.

Among the alternatives listed so far, I still prefer the one we use in the other methods, although it is 2 lines, not only 1.

Currently it compiles, but I'm sure newer CLANG versions will soon recognize that it is the same as

return *(reinterpret_cast<Value *>(0));

and will complain about it.

Anyway, these methods in concept classes need not be run, only compiled. So what about throwing an exception instead of such pointer tricks?

Good idea. It may work. Hopefully it wont complain that a non void function has no return.

On the other hand, we must be careful, because - if I remember correctly - some codes do call concept checking functions directly.

[Peter Kovacs]

Replying to Alpar Juttner:

As I explained, sizeof(Value) is not just an arbitrary value. It is used because is ensures the proper alignment of the underlying pointer.

OK, I understand now and accept your solution.

On the other hand, we must be careful, because - if I remember correctly - some codes do call concept checking functions directly.

If these methods are actually run (not only compiled), then none of the suggested solutions seem to be safe. Throwing an exception at least makes the issue explicit and clear (instead of a "nice" segmentation fault). Provided that compilers do not give warnings for that.

[Alpar Juttner]

The fix is in the main branch as [a278d16bd2d0]