COIN-OR::LEMON - Graph Library

Opened 4 years ago

Closed 3 years ago

#634 closed defect (fixed)

Clang related fix

Reported by: Alpar Juttner Owned by: Alpar Juttner
Priority: critical Milestone: LEMON 1.4 release
Component: core Version: hg main
Keywords: Cc:
Revision id:

Description

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?

Attachments (1)

clang-fix.patch (671 bytes) - added by Alpar Juttner 4 years ago.

Download all attachments as: .zip

Change History (11)

Changed 4 years ago by Alpar Juttner

Attachment: clang-fix.patch added

comment:1 Changed 4 years ago by Alpar Juttner

Priority: majorcritical

comment:2 Changed 4 years ago by 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));
}

comment:3 Changed 4 years ago by Alpar Juttner

It seems that

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

also works, and it may even be safer.

comment:4 in reply to:  3 ; Changed 4 years ago by 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.

comment:5 in reply to:  4 Changed 4 years ago by 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.

comment:6 Changed 4 years ago by 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)));
                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

comment:7 Changed 4 years ago by 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?

comment:8 in reply to:  7 ; Changed 4 years ago by 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.

comment:9 in reply to:  8 Changed 4 years ago by 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.

comment:10 Changed 3 years ago by Alpar Juttner

Resolution: fixed
Status: newclosed

The fix is in the main branch as [a278d16bd2d0]

Note: See TracTickets for help on using tickets.