Re: type punning in gl_list.hh

Re: type punning in gl_list.hh

[Bruno Haible]

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  <bruno@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); }

Re: iterator and const iterator

[Bruno Haible]

Hi Akim,

In <https://lists.gnu.org/archive/html/bug-bison/2020-05/msg00102.html> you
write:
> > Le 3 mai 2020 à 17:17, Bruno Haible <bruno@clisp.org> a écrit :
> > 
> > There are a couple of GCC warnings on IRIX (CC="gcc -mabi=n32"):
> > 
> > 2)
> > ../src/fixits.c: In function `fixits_run':
> > ../src/fixits.c:129: warning: dereferencing type-punned pointer will break strict-aliasing rules
> > 
> > Modern C rules wants f to be a 'const void *'. Inside the loop body,
> > you can cast f to 'fixit const *' without danger.
> 
> So I had installed this:
> 
> -  fixit const *f = NULL;
> +  void const *p = NULL;
>    gl_list_iterator_t iter = gl_list_iterator (fixits);
> -  while (gl_list_iterator_next (&iter, (const void**) &f, NULL))
> +  while (gl_list_iterator_next (&iter, &p, NULL))
>      {
> +      fixit const *f = p;
> 
> 
> 
> Today we have more occurrences of this pattern that we have to fix,
> but some of these items need the pointer to point to a non-const value,
> so we need a const void * pointer, and still a cast.
> 
> Would you accept a macro, say gl_list_mutable_iterator_next, where we
> would pass a void** instead of const void**?

No, I don't want this. It's a madness, in C++, to have to write the same
member function definition twice, due to 'const' differences. This madness
should not propagate into our C code. In C, we don't need to have two
functions which produce the exactly same machine code. C has sufficient
means for guaranteeing that one copy of the code will do.

> Of course mutable_iterator
> gives the impression that it's the iterator which is mutable, but an
> iterator must so obviously be mutable that I don't think it would be
> much of an issue, and it's quite symmetrical with const_iterator.
> It might have been less surprising to have the current gl_list_iterator_next
> be about mutable values and use gl_list_const_iterator_next for readonly
> iteration, but it's too late.

Whether you call it mutable_iterator vs. iterator, or iterator vs.
const_iterator, doesn't change the fact that it's a code duplication.

Bruno