Re: [boost] [serialization] patch for portable_binary_oarchive.hpp

From: David Abrahams (dave_at_[hidden])
Date: 2007-11-02 22:28:09

on Fri Nov 02 2007, "Boris Gubenko" <Boris.Gubenko-AT-hp.com> wrote:

> David Abrahams wrote:
>> I can't imagine how your change would fix an endianness problem.
>
> It does not fix "an endianness problem". The bug is in the code
> conditionalized with '#ifdef BOOST_BIG_ENDIAN'. This is the only
> connection between endianness and the fix.
>
>> Also, your change relies on the nonportable (implementation-defined)
>> behavior of reinterpret_cast.
>
> It does. As well as other Boost code using reinterpret_cast :-)
>
> Scanning Boost sources (trunk) for reinterpret_cast yields 700 hits,
> including 8 instances in the Boost python library (two of them are
> commented out):

Those are most likely all wrong; they should be looked at one by one
and fixed.

> bash-2.03$ find ./boost/libs/python -type f -exec grep reinterpret_cast {} \;
>       instance<>* self = reinterpret_cast<instance<>*>(inst);
>         _doc += str(reinterpret_cast<const char*>(detail::py_signature_tag));
> //            _doc += "\n"+str(reinterpret_cast<const 
> char*>(detail::cpp_signature_tag));
>             _doc += str(reinterpret_cast<const 
> char*>(detail::cpp_signature_tag));
>       instance<>* self = reinterpret_cast<instance<>*>(inst);
>         _doc += str(reinterpret_cast<const char*>(detail::py_signature_tag));
> //            _doc += "\n"+str(reinterpret_cast<const 
> char*>(detail::cpp_signature_tag));
>             _doc += str(reinterpret_cast<const 
> char*>(detail::cpp_signature_tag));
> bash-2.03$

I am apparently responsible for one of those, though only a careful
combing through history will tell for sure.  I need to bother other
people about the other instances.

> You are, probably, referring to 5.2.10 - Reinterpret cast
> [expr.reinterpret.cast], para 3:
>
>  -3- The mapping performed by reinterpret_cast is implementation-defined.
>      [Note: it might, or might not, produce a representation different
>      from the original value. ]
>
> In my fix, reinterpret_cast performs a mapping from non-null pointer to
> long to pointer to char. If reinterpret_cast cannot be used in this case
> without invoking implementation-defined behavior, I'm not sure how it can
> be used in a multiplatform code like Boost at all.

Very perceptive.  There are almost no ways to use reinterpret_cast
portably.  If you want to get a char* that traverses the bytes of a
long, you do it as below:

>> I think the correct change to silence the type conversion error is
>>
>>   char * first = static_cast<char *>(static_cast<void *>(const_cast<long*>(& 
>> l)));
>
> I'm not sure my change is incorrect. 

It is if you're trying to write portable code.  You've already found
the place in the standard where it says reinterpret_cast performs an
implementation-defined mapping.  If you want to assert your change is
portable, please show me where in the standard it defines the behavior
of your specific usage.

> In terms of style, I think that your
> change is better.

Hah, I think it's stylistically distasteful, but at least it's
correct.  I wish that it were properly spelled
reinterpret_cast<char*>(&l) ;-)

-- 
Dave Abrahams
Boost Consulting
http://www.boost-consulting.com