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

From: Boris Gubenko (Boris.Gubenko_at_[hidden])
Date: 2007-11-02 18:42:04

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):

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$

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.

> 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. In terms of style, I think that your
change is better.

Thanks,
  Boris

----- Original Message ----- 
From: "David Abrahams" <dave_at_[hidden]>
To: <boost_at_[hidden]>
Sent: Friday, November 02, 2007 4:33 PM
Subject: Re: [boost] [serialization] patch for portable_binary_oarchive.hpp

>
> on Fri Nov 02 2007, "Boris Gubenko" <Boris.Gubenko-AT-hp.com> wrote:
>
>> Two serialization library tests: test_demo_portable_archive[_dll] fail
>> to compile on many platforms with invalid type conversion error. I did
>> not check all the platforms, but I'd expect it to fail on any big endian
>> platform.
>
> I can't imagine how your change would fix an endianness problem.
> Also, your change relies on the nonportable (implementation-defined)
> behavior of reinterpret_cast.
>
> I think the correct change to silence the type conversion error is
>
>  char * first = static_cast<char *>(static_cast<void *>(const_cast<long*>(& 
> l)));
>
> However the code in itself invokes undefined behavior by modifying a
> constant object (l).  The byte reversal needs to be done in a separate
> buffer.
>
>> The patch below fixes the problem, verified on HP-UX with
>> both aC++ and gcc.
>>
>> Ok to commit?
>>
>> Index: example/portable_binary_oarchive.hpp
>> ===================================================================
>> --- example/portable_binary_oarchive.hpp        (revision 40687)
>> +++ example/portable_binary_oarchive.hpp        (working copy)
>> @@ -76,7 +76,7 @@
>>
>>          // we choose to use litle endian
>>          #ifdef BOOST_BIG_ENDIAN
>> -            char * first = static_cast<char *>(static_cast<void *>(& l));
>> +            char * first = reinterpret_cast<char *>(const_cast<long *>(& 
>> l));
>>              char * last = first + sizeof(l) - 1;
>>              for(;first < last;++first, --last){
>>                  char x = *last;
>>
>> Thanks,
>>   Boris
>>
>> _______________________________________________
>> Unsubscribe & other changes: 
>> http://lists.boost.org/mailman/listinfo.cgi/boost
>>
>
> -- 
> Dave Abrahams
> Boost Consulting
> http://www.boost-consulting.com
>
> _______________________________________________
> Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
>