Akim Demaille wrote in <https://lists.gnu.org/archive/html/bug-bison/2020-05/msg00102.html>: > Also, in gnulib, I found this in gl_list.hh: > > /* If there is a next element, stores the next element in ELT, advances > the iterator and returns true. > Otherwise, returns false. */ > bool next (ELTYPE *& elt) > { return gl_list_iterator_next (&_state, reinterpret_cast<const void > **>(&elt), NULL); } > > /* If there is a next element, stores the next element in ELT, stores its > node in *NODEP if NODEP is non-NULL, advances the iterator and returns > true. > Otherwise, returns false. */ > bool next (ELTYPE *& elt, gl_list_node_t *nodep) > { return gl_list_iterator_next (&_state, reinterpret_cast<const void > **>(&elt), nodep); } > > > It is not my understanding that reinterpret_cast protects from type > punning issues. Did I miss something? We've had in the past to > do something alike in Bison > (https://lists.gnu.org/r/bison-patches/2013-01/msg00131.html).
Agreed. 10 years ago, this reinterpret_cast was probably safe, but nowadays with the more and more aggressive optimizations of the C++ compilers, such a reinterpret_cast is dangerous. I'm applying this patch. Let's hope that the C++ compilers can merge the two tests of the same boolean value into one, and thus not lose too much performance. 2020-05-31 Bruno Haible <br...@clisp.org> list-c++, set-c++, oset-c++, map-c++, omap-c++: Don't fool the compiler. Reported by Akim Demaille in <https://lists.gnu.org/archive/html/bug-bison/2020-05/msg00102.html>. * lib/gl_list.hh (gl_List::iterator::next): Avoid a reinterpret_cast. * lib/gl_set.hh (gl_Set::iterator::next): Likewise. * lib/gl_oset.hh (gl_OSet::iterator::next): Likewise. * lib/gl_map.hh (gl_Map::iterator::next): Likewise. * lib/gl_omap.hh (gl_OMap::iterator::next): Likewise. diff --git a/lib/gl_list.hh b/lib/gl_list.hh index b19bda7..8b0ccad 100644 --- a/lib/gl_list.hh +++ b/lib/gl_list.hh @@ -351,13 +351,25 @@ public: the iterator and returns true. Otherwise, returns false. */ bool next (ELTYPE *& elt) - { return gl_list_iterator_next (&_state, reinterpret_cast<const void **>(&elt), NULL); } + { + const void *next_elt; + bool has_next = gl_list_iterator_next (&_state, &next_elt, NULL); + if (has_next) + elt = static_cast<ELTYPE *>(next_elt); + return has_next; + } /* If there is a next element, stores the next element in ELT, stores its node in *NODEP if NODEP is non-NULL, advances the iterator and returns true. Otherwise, returns false. */ bool next (ELTYPE *& elt, gl_list_node_t *nodep) - { return gl_list_iterator_next (&_state, reinterpret_cast<const void **>(&elt), nodep); } + { + const void *next_elt; + bool has_next = gl_list_iterator_next (&_state, &next_elt, nodep); + if (has_next) + elt = static_cast<ELTYPE *>(next_elt); + return has_next; + } ~iterator () { gl_list_iterator_free (&_state); } diff --git a/lib/gl_map.hh b/lib/gl_map.hh index f3d0a46..082694f 100644 --- a/lib/gl_map.hh +++ b/lib/gl_map.hh @@ -143,7 +143,17 @@ public: /* If there is a next pair, stores the next pair in KEY and VALUE, advance the iterator, and returns true. Otherwise, returns false. */ bool next (KEYTYPE *& key, VALUETYPE *& value) - { return gl_map_iterator_next (&_state, reinterpret_cast<const void **>(&key), reinterpret_cast<const void **>(&value)); } + { + const void *next_key; + const void *next_value; + bool has_next = gl_map_iterator_next (&_state, &next_key, &next_value); + if (has_next) + { + key = static_cast<KEYTYPE *>(next_key); + value = static_cast<VALUETYPE *>(next_value); + } + return has_next; + } ~iterator () { gl_map_iterator_free (&_state); } diff --git a/lib/gl_omap.hh b/lib/gl_omap.hh index 15d8104..1f892e4 100644 --- a/lib/gl_omap.hh +++ b/lib/gl_omap.hh @@ -150,7 +150,17 @@ public: /* If there is a next pair, stores the next pair in KEY and VALUE, advances the iterator, and returns true. Otherwise, returns false. */ bool next (KEYTYPE *& key, VALUETYPE *& value) - { return gl_omap_iterator_next (&_state, reinterpret_cast<const void **>(&key), reinterpret_cast<const void **>(&value)); } + { + const void *next_key; + const void *next_value; + bool has_next = gl_omap_iterator_next (&_state, &next_key, &next_value); + if (has_next) + { + key = static_cast<KEYTYPE *>(next_key); + value = static_cast<VALUETYPE *>(next_value); + } + return has_next; + } ~iterator () { gl_omap_iterator_free (&_state); } diff --git a/lib/gl_oset.hh b/lib/gl_oset.hh index 16da0dd..b78ef52 100644 --- a/lib/gl_oset.hh +++ b/lib/gl_oset.hh @@ -121,7 +121,13 @@ public: /* If there is a next element, stores the next element in ELT, advances the iterator and returns true. Otherwise, returns false. */ bool next (ELTYPE *& elt) - { return gl_oset_iterator_next (&_state, reinterpret_cast<const void **>(&elt)); } + { + const void *next_elt; + bool has_next = gl_oset_iterator_next (&_state, &next_elt); + if (has_next) + elt = static_cast<ELTYPE *>(next_elt); + return has_next; + } ~iterator () { gl_oset_iterator_free (&_state); } diff --git a/lib/gl_set.hh b/lib/gl_set.hh index 357e141..3b64bc5 100644 --- a/lib/gl_set.hh +++ b/lib/gl_set.hh @@ -114,7 +114,13 @@ public: /* If there is a next element, stores the next element in ELT, advances the iterator and returns true. Otherwise, returns false. */ bool next (ELTYPE *& elt) - { return gl_set_iterator_next (&_state, reinterpret_cast<const void **>(&elt)); } + { + const void *next_elt; + bool has_next = gl_set_iterator_next (&_state, &next_elt); + if (has_next) + elt = static_cast<ELTYPE *>(next_elt); + return has_next; + } ~iterator () { gl_set_iterator_free (&_state); }