Re: [boost] [endian] Some suggestions

Subject: Re: [boost] [endian] Some suggestions
From: Beman Dawes (bdawes_at_[hidden])
Date: 2016-04-13 08:14:31

On Mon, Apr 11, 2016 at 8:54 AM, Tatsuyuki Ishi <ishitatsuyuki_at_[hidden]>
wrote:

> > > 1. Bring back float: make a proper reason for abandon
> > > You told about NaN: but it shouldn't apply since you store it in char
> > > buffer.
> > >
> >
> > The problem isn't when storing into the char buffer, but later on a
> > different machine or compiler when the char buffer has to be converted
> to a
> > float type.
>
> I know nothing other than float format differences, and my target
> platforms are using iec559. I think reinterpret_cast should work: char
> data shouldn't be changed as long as stored as char.
>

Yes. I should have said that the problem isn't when storing into a char
buffer, as is done by the endian_buffer and endian_arithmetic classes, but
in the conversion functions where possibly byte-swapped values are passed
as float or double:

          double endian_reverse(double);

That's the interface I'm concerned about because of the return of a double
that in fact is just an array of 4 bytes.

>
> >
> > In testing, I even ran into a case were on the same machine and compiler
> > the float data failed to round-trip correctly if the program that created
> > the file was compiled with a different set of optimization options that
> the
> > program the read the file.
> >
> I will be happy if you provide some example.
>

See my reply to Peter Dimov.

> >
> > >
> > > 2. Remove n_bits(endian buffers): completely useless
> > > This just look like an alternative to sizeof, and always /8'd when
> used.
> > > What's the meaning of that?
> > >
> >
> > Please give a specific code example from the docs of what you think
> should
> > be changed. Or submit a pull request.
> >
> Since this is breaking API, I'd like you to make the decision to
> change it or not.
>
> For detail, this is the endian_buffer definition:
> template <order Order, class T, std::size_t n_bits,
> align A = align::no>
> class endian_buffer;
>
> n_bits looks quite useless here. It's basically sizeof*8.
>

The size has to be supplied as either the size in bytes or the size in
bits. Remember that these classes support 3, 5, 6, and 7 byte integers as
well as the more usual 1, 2, 4, and 8 byte integers.

>
> >
> > >
> > > 3. Minor API suggestions
> > > Move operator type to endian_buffer: it's not related to arithmetic but
> > > just used because it can act like an integer.
> > >
> >
> > Again, what code is it you think should be changed? Operator
> value_type()?
> > Please submit a pull request.
> >
>
> Exactly: operator value_type(). This will provide much more convenience.
> An example: when you specify the size for the array:
> endian_buffer size;
> dynarray d(size.value()); vs dynarray d(size);
>

The rationale for a separate endian_buffer class was that some formal
review comments were very concerned about the expense of conversions in
class endian_arithmetic. They asked for a separate endian_buffer class that
did not perform arithmetic and other automatic conversions. To add
automatic conversions to endian_buffer would partially negate the whole
purpose of a separate endian_buffer class.

>
> >
> > > Non-const data(): I'm tired to do reinterpret_cast. Look at C++11
> vector
> > > for a reference.
> > >
> >
> > What is the motivation? What are the safety implications? What are the
> > alternatives?  The C++11 (and also C++17) addition of non-cost data()
> > members to some standard library containers was supported by motivation
> and
> > safety analysis.
> >
>
> Seems endian_buffer is targeted for using in structs. Then is there
> any problem to provide a wrap of reinterpret_cast? At least, it should
> be safe by design.
>

I worry about buffer overflows. (I also worried about that problem for the
standard library. Only time will tell if that worry is well founded.)

> >
> > > Move stream operators to endian_arithmetic: I first thought that was
> for
> > > reading binary. That's basically unused.
> > >
> >
> > I must be missing something. Why wouldn't stream operators apply to
> endian
> > buffers? A code example might help.
> >
> Quite confusing. I first thought that was binary r/w operators that
> reads/writes the stream and store the value.
>
> The problem is that, there's no method provided for reading from
> binary, so this is also a reason for the previous data() suggestion.
>

I agree there is a need to better handle binary I/O, but it seems to me
that is a problem with istream/ostream from the standard library rather
than the endian library.

Thanks,

--Beman