Re: [PATCH] variants: avoid type punning issue.

[Akim Demaille]

Le 29 janv. 2013 à 22:59, Theophile Ranquet <address@hidden> a écrit :

> This is based on what Scott Meyers recommends in 'Effective C++'.

Actually it's Alexandrescu and Sutter in "C++ Coding Standards"
that I showed to you :)  And there is quite a few threads about
this on StackOverflow.

http://stackoverflow.com/questions/573294/when-to-use-reinterpret-cast
http://stackoverflow.com/questions/1863069/casting-via-void-instead-of-using-reinterpret-cast?lq=1
http://stackoverflow.com/questions/5025843/why-do-we-have-reinterpret-cast-in-c-when-two-chained-static-cast-can-do-its
etc.

> Use a static_cast on void* rather than directly use a reinterpret_cast, which
> can have nefarious effects on objects.  However, even though following this
> guideline is good practice in general, I am not quite sure how relevant it is
> when applied to conversions from POD to objects.  Actually, it might very well
> be the opposite: isn't this exactly what reinterpret_cast is for? What we
> really want *is* to transmit the memory map as a series of bytes, which, if I
> am correct, falls into the kind of "low level" hack for which this cast is
> meant.

I agree.

> In any case, this silences the warning, which will be greatly appreciated by
> anyone using variants with a compiler supporting -fstrict-aliasing.

Well, the point is not to silence the warning, but really
to stop worrying the compiler about what we are doing here.
The reinterpret_cast on the raw memory was fine, except
that G++ could lose track of what we were doing with our
object, which is bad.  I'm looking for any means to avoid this.
I would have preferred to keep the reinterpret_cast, which is
more faithful here (we are really reading the memory with the
different pattern of interpretation, as you noted above) and
with the GCC attribute "may_alias"
(http://gcc.gnu.org/onlinedocs/gcc-4.7.0/gcc/Type-Attributes.html)
but we failed to use it to fix the issue, right?

> * data/variant.hh (as): Here.
> * tests/c++.at (Exception safety): Revert commit ddb9db15, type punning
> is no longer an issue.

This is not the only place where NO_STRICT_ALIAS_CXXFLAGS is
used: please remove:

- all its uses
- all the code that defines it
- references to this type punning issue in the documentation

> ---
> data/variant.hh | 10 ++++++++--
> tests/c++.at    |  2 +-
> 2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/data/variant.hh b/data/variant.hh
> index 047e641..e2a537a 100644
> --- a/data/variant.hh
> +++ b/data/variant.hh
> @@ -142,7 +142,10 @@ m4_define([b4_variant_define],
>     {]b4_parse_assert_if([
>       YYASSERT (tname == typeid (T).name ());
>       YYASSERT (sizeof (T) <= S);])[
> -      return reinterpret_cast<T&> (buffer.raw);
> +      {
> +        void *dummy = buffer.raw;
> +        return *static_cast<T*> (dummy);
> +      }
>     }
> 
>     /// Const accessor to a built \a T (for %printer).
> @@ -152,7 +155,10 @@ m4_define([b4_variant_define],
>     {]b4_parse_assert_if([
>       YYASSERT (tname == typeid (T).name ());
>       YYASSERT (sizeof (T) <= S);])[
> -      return reinterpret_cast<const T&> (buffer.raw);
> +      {
> +        const void *dummy = buffer.raw;
> +        return *static_cast<const T*> (dummy);
> +      }
>     }
> 
>     /// Swap the content with \a other, of same type.
> diff --git a/tests/c++.at b/tests/c++.at
> index a21fb4e..874d640 100644
> --- a/tests/c++.at
> +++ b/tests/c++.at
> @@ -804,7 +804,7 @@ main (int argc, const char *argv[])
> }
> ]])
> AT_BISON_CHECK([[-o input.cc --report=all input.yy]])
> -AT_COMPILE_CXX([input], [[$NO_STRICT_ALIAS_CXXFLAGS input.cc]])
> +AT_COMPILE_CXX([[input]])
> 
> AT_PARSER_CHECK([[./input aaaas]], [[2]], [[]],
> [[exception caught: reduction
> -- 
> 1.8.1.2
> 
>