[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: type punning in gl_list.hh
From: |
Bruno Haible |
Subject: |
Re: type punning in gl_list.hh |
Date: |
Sun, 31 May 2020 12:07:54 +0200 |
User-agent: |
KMail/5.1.3 (Linux/4.4.0-177-generic; KDE/5.18.0; x86_64; ; ) |
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); }