[Endian] Review

classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

[Endian] Review

Tomas Puverle
Hi Beman,

>      - What is your evaluation of the design?

The design of the integer types seems reasonable to me, even though I would
prefer not to have the overloaded operators on them.  I feel that having the
operators has the potential to break the boundary between program input and
the program's processing.  What do you I mean by that?  Typically, I try to
structure my programs as follows:

        1) Read input data
        2) Verify input invariants, construct internal representation
        3) Process internal representation
        4) Output result

The problem with having operators in the endian types is that they encourage
the user to continue using them past the point 2).

However, as I said, that's my personal preference and last year plenty of
people wanted to see the operators included.  Perhaps they could be a policy?  

endian<..., bool EnableOperators_ > ?

As for the rest of the library, I have the following comments:

- As many other people have mentioned, floating point support and user defined
types are a must.
- I am happy that the "conversion functions" have been added, Unfortunately,
I find them lacking in several departments:
        - (Note: I am going to try very hard not to get drawn into another naming
                discussion.  That's pretty much what ended my efforts last year -
                50 people commenting, each proposing a different set of names :D )
        - While the endian types support unaligned data, the endian functions
                don't appear to do so.  I don't see how the endian types could be
                implemented using them.
        - No support for user defined types, arrays/ranges of user defined types
          etc.  This is extremely useful.  A frequent example I come across is
          the following (using our syntax):
                        //load a file into memory
                        //...
                        swapInPlace<BigToNative>(data.begin(), data.end());
       
          Depending on the platform, that last statement may get compiled out
          to a no-op.  This is also the big reason why our version of "reorder()"
          is not unconditional but depends on the "swap type".
       
        - reorder(x) will work fine for integral types, however, has potential
          to cause issues for e.g. floating types.  This has been discussed
          extensively during my proposal last year.  Also, from my practical
          experience, having a floating point number as an input, I find
          the assembly typically includes (unncecessary) floating point
          load/store instructions.  Something along the lines of e.g.:
               
                template<typename T_>
                T_ big_to_native(raw_memory<T_>)
               
                and
               
                template<typename T_>
                swapped_data<T_> native_to_big(T_)
               
          might be better and result in better performance.  Of course, those
          two function signatures don't solve the "reorder" signature, but that
          could be changed correspondingly.
        - reorder() should use platform specific instructions (such as bswap)
          for doing endian swapping, otherwise falling back on one of the (many)
          variants which are currently being discussed.
        - I think the alternatives such as little_to_big() and big_to_little()
          are also needed.
        - I would like to see a few standard traits/typedefs so that I don't have
          to keep checking pre-processor macros, such as
       
                typedef boost::mpl::true_/false_ platform_is_big_endian;
               
          or something similar.
         
        - While I haven't needed this myself, last year someone mentioned the need
          for a "general reorder" functionality.  Rather than simply dealing with
          little/big endianness, allow the user to specify ANY byte ordering for
          the specific data size.

- I would like to see an endian_swap_iterator (and a raw_endian_iterator,
  which can iterate over unaligned data, too)

>      - What is your evaluation of the implementation?

Other than the implementation of reorder() and its performance, which is
already being discussed on the list, (and the inclusion of platform specific
instructions, as per above) I would like to see endian<> to be implemented
in terms of the "free" functions.  It seems strange to have two implementations
of the same task.

>      - What is your evaluation of the documentation?

Fine.

>      - What is your evaluation of the potential usefulness of the
>        library?

Very useful, long overdue in boost.

>      - Did you try to use the library?  With what compiler?  Did you
>        have any problems?

Tried the integer types last year, no problems.  Didn't try the "new"
swapping functions.


>      - How much effort did you put into your evaluation? A glance? A
>        quick reading? In-depth study?

I spent a fair bit of time looking through the docs, the code and reading
the followup posts etc.

>      - Are you knowledgeable about the problem domain?

Reasonably.

> And finally, every review should answer this question:
>
>      - Do you think the library should be accepted as a Boost library?

I see the library as having two pieces, both useful to different people.
        I would say conditionally accept the endian<> class if floating point
        support is added.
        I would vote no for the inclusion of the free function interfaces in
        their current form.


Thanks!

Tom





_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Endian] Review

Beman Dawes
On Thu, Sep 8, 2011 at 1:18 PM, Tomas Puverle <
[hidden email]> wrote:

> Hi Beman,
>
> >      - What is your evaluation of the design?
>
> The design of the integer types seems reasonable to me, even though I would
> prefer not to have the overloaded operators on them.  I feel that having
> the
> operators has the potential to break the boundary between program input and
> the program's processing.  What do you I mean by that?  Typically, I try to
> structure my programs as follows:
>
>        1) Read input data
>        2) Verify input invariants, construct internal representation
>        3) Process internal representation
>        4) Output result
>

> The problem with having operators in the endian types is that they
> encourage
> the user to continue using them past the point 2).
>

I'm in complete agreement with you for large and/or performance critical
programs need careful structuring. But not all programs are large and/or
performance critical, so the convenience of having the full range of
operations triumphs over other considerations. The conversion functions are
available where the "input the data, do all the incoming byte reordering, do
all the processing, do all the outgoing byte reordering, output the data"
model is desired.

But there can be other considerations. Program flow is sometimes chaotic,
and then it become easy to become confused over which variables need to be
reordered and which don't. The endian integer approach is much less
error-prone in such situations.


>
> However, as I said, that's my personal preference and last year plenty of
> people wanted to see the operators included.  Perhaps they could be a
> policy?
>
> endian<..., bool EnableOperators_ > ?
>

I'll give it some thought, but my initial reaction is that endian is already
more complex than I like, so I'm leery of adding yet more complexity.


>
> As for the rest of the library, I have the following comments:
>
> - As many other people have mentioned, floating point support and user
> defined
> types are a must.
>

I think floating point is easily doable, although details haven't been
worked out yet.

Do you have an example of a UDT that you would like to see work? And what do
you want it to work for? Both class endian and the conversion functions?


> - I am happy that the "conversion functions" have been added,
> Unfortunately,
> I find them lacking in several departments:
>        - (Note: I am going to try very hard not to get drawn into another
> naming
>                discussion.  That's pretty much what ended my efforts last
> year -
>                50 people commenting, each proposing a different set of
> names :D )
>

Understood:-)

       - While the endian types support unaligned data, the endian functions
>                don't appear to do so.  I don't see how the endian types
> could be
>                implemented using them.
>

Right. That would be a highly desirable addition.


>        - No support for user defined types, arrays/ranges of user defined
> types
>          etc.  This is extremely useful.  A frequent example I come across
> is
>          the following (using our syntax):
>                        //load a file into memory
>                        //...
>                        swapInPlace<BigToNative>(data.begin(), data.end());
>
>          Depending on the platform, that last statement may get compiled
> out
>          to a no-op.  This is also the big reason why our version of
> "reorder()"
>          is not unconditional but depends on the "swap type".
>

Yep, that's why the conditional reodering functions are provided. The names
are a serious impediment to understanding. Maybe they all should include
"reorder" and "conditional" in the names for clarity.

The other issue that has come up is the interface. A pure function (i.e.
returning the new value) interface would allow use of std::transform. Not
sure how that would play out with unaligned or unusual sized types, however.

I'm glazing over, so will wait to reply to the rest of your comments. I'll
also dig out your previous proposal and review the floating point issues you
mention.

Thanks!

--Beman

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Endian] Review

Steven Watanabe-4
AMDG

On 09/08/2011 12:23 PM, Beman Dawes wrote:

> On Thu, Sep 8, 2011 at 1:18 PM, Tomas Puverle <
> [hidden email]> wrote:
>
>>
>> However, as I said, that's my personal preference and last year plenty of
>> people wanted to see the operators included.  Perhaps they could be a
>> policy?
>>
>> endian<..., bool EnableOperators_ > ?
>>
>
> I'll give it some thought, but my initial reaction is that endian is already
> more complex than I like, so I'm leery of adding yet more complexity.
>
If you don't want the operators, then don't use
them.  Having an extra template parameter to
disable them seems rather pointless.

In Christ,
Steven Watanabe



_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

signature.asc (501 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [Endian] Review

Vicente Botet
Steven Watanabe-4 wrote
AMDG

On 09/08/2011 12:23 PM, Beman Dawes wrote:
> On Thu, Sep 8, 2011 at 1:18 PM, Tomas Puverle <
> [hidden email]> wrote:
>
>>
>> However, as I said, that's my personal preference and last year plenty of
>> people wanted to see the operators included.  Perhaps they could be a
>> policy?
>>
>> endian<..., bool EnableOperators_ > ?
>>
>
> I'll give it some thought, but my initial reaction is that endian is already
> more complex than I like, so I'm leery of adding yet more complexity.
>

If you don't want the operators, then don't use
them.  Having an extra template parameter to
disable them seems rather pointless.
Hi,

I have separated the endian class into a endian_pack that is integer agnostic and an endian class that behaves as an integer (You could see the details in the Sandbox under endian_ext).

Reference docs:
http://svn.boost.org/svn/boost/sandbox/endian_ext/libs/integer/endian/doc/html/toward_boost_integer_endian/reference.html#toward_boost_integer_endian.reference.integer_endian_pack_hpp

http://svn.boost.org/svn/boost/sandbox/endian_ext/libs/integer/endian/doc/html

(Sorry but there is a bad link to the reference)

This is not complex and allows to define physical structures that can be used only at the I/O frontier.

To separate the concerns and avoid the inefficient arithmetic on endian integers, the example posted on another thread

struct file_header {
   big32_t n_things;
   ....
};

int main()
{
   file_header h;
   h.n_things = 0;
   while (....) {
     ++h.n_things;
     ....
   }
   write(h);
   ....
}

could be rewritten as follows to force more efficient code

struct file_header {
   big32_pt n_things;
   ....
};

where big32_pt is a typedef for a 32 bits
typedef endian_pack<endianness::big, int_least32_t, 32, alignment::unaligned> big32_pt;

int main()
{
   file_header h;
   int n_things = 0;
   while (....) {
     ++n_things;
     ....
   }
   h.n_things = n_things;
   write(h);
   ....
}

Best,
Vicente
Reply | Threaded
Open this post in threaded view
|

Re: [Endian] Review

Beman Dawes
On Fri, Sep 9, 2011 at 2:32 AM, Vicente Botet <[hidden email]>wrote:

> Hi,
>
> I have separated the endian class into a endian_pack that is integer
> agnostic and an endian class that behaves as an integer (You could see the
> details in the Sandbox under endian_ext).
>
> Reference docs:
>
> http://svn.boost.org/svn/boost/sandbox/endian_ext/libs/integer/endian/doc/html/toward_boost_integer_endian/reference.html#toward_boost_integer_endian.reference.integer_endian_pack_hpp
>
>
> http://svn.boost.org/svn/boost/sandbox/endian_ext/libs/integer/endian/doc/html
>
> (Sorry but there is a bad link to the reference)
>
> This is not complex


Complexity is in the eye of the beholder. To me, that approach was markedly
more complex and confusing.


> and allows to define physical structures that can be
> used only at the I/O frontier.
>

See my reply to Phil Endecott (this morning, 9/9/11) for an example of doing
this.

--Beman

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Endian] Review

Tomas Puverle
In reply to this post by Steven Watanabe-4
> If you don't want the operators, then don't use
> them.  Having an extra template parameter to
> disable them seems rather pointless.

Steven,

That, in my view, is the same argument as saying that "a bidirectional iterator
should have an operator +=, but just don't use it".

Why doesn't the STL provide it?  Because it would lead to surprises when people
expecting a quick operation end up getting something much more expensive.  While
the algorithmic complexity of the endian operators may not quite cause the
difference between O(1) and O(N), I think it's teetering in the same territory.
Cheers,

Tom


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Endian] Review

Steven Watanabe-4
AMDG

On 09/09/2011 07:59 AM, Tomas Puverle wrote:
>> If you don't want the operators, then don't use
>> them.  Having an extra template parameter to
>> disable them seems rather pointless.
>
> Steven,
>
> That, in my view, is the same argument as saying that "a bidirectional iterator
> should have an operator +=, but just don't use it".
>

It isn't the same at all.  I was really
responding to the suggestion of having
a template parameter that determines whether
they should be provided.  I don't really
have an opinion on whether the operators
should be provided or not.  IMHO, this
half-way thing of optionally providing them
is liable to cause more confusion than
always providing them.

> Why doesn't the STL provide it?  Because it would lead to surprises when people
> expecting a quick operation end up getting something much more expensive.  While
> the algorithmic complexity of the endian operators may not quite cause the
> difference between O(1) and O(N), I think it's teetering in the same territory.
>

In Christ,
Steven Watanabe



_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

signature.asc (501 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [Endian] Review

Vicente Botet
In reply to this post by Beman Dawes
Le 09/09/11 15:05, Beman Dawes a écrit :

> On Fri, Sep 9, 2011 at 2:32 AM, Vicente Botet<[hidden email]>wrote:
>
>> Hi,
>>
>> I have separated the endian class into a endian_pack that is integer
>> agnostic and an endian class that behaves as an integer (You could see the
>> details in the Sandbox under endian_ext).
>>
>> Reference docs:
>>
>> http://svn.boost.org/svn/boost/sandbox/endian_ext/libs/integer/endian/doc/html/toward_boost_integer_endian/reference.html#toward_boost_integer_endian.reference.integer_endian_pack_hpp
>>
>>
>> http://svn.boost.org/svn/boost/sandbox/endian_ext/libs/integer/endian/doc/html
>>
>> (Sorry but there is a bad link to the reference)
>>
>> This is not complex
>
> Complexity is in the eye of the beholder. To me, that approach was markedly
> more complex and confusing.
>
What do you find complex in separating the classes endian_pack and endian?
>> and allows to define physical structures that can be
>> used only at the I/O frontier.
>>
> See my reply to Phil Endecott (this morning, 9/9/11) for an example of doing
> this.
>
Well, IIUC your replay doesn't force the separation of concerns. Of
course, the user of such a structure could do the separation, but the
interface doesn't force it.

Best,
Vicente


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Endian] Review

Beman Dawes
On Fri, Sep 9, 2011 at 3:20 PM, Vicente J. Botet Escriba
<[hidden email]> wrote:

> Le 09/09/11 15:05, Beman Dawes a écrit :
>>
>> On Fri, Sep 9, 2011 at 2:32 AM, Vicente
>> Botet<[hidden email]>wrote:
>>
>>> Hi,
>>>
>>> I have separated the endian class into a endian_pack that is integer
>>> agnostic and an endian class that behaves as an integer (You could see
>>> the
>>> details in the Sandbox under endian_ext).
>>>
>>> Reference docs:
>>>
>>>
>>> http://svn.boost.org/svn/boost/sandbox/endian_ext/libs/integer/endian/doc/html/toward_boost_integer_endian/reference.html#toward_boost_integer_endian.reference.integer_endian_pack_hpp
>>>
>>>
>>>
>>> http://svn.boost.org/svn/boost/sandbox/endian_ext/libs/integer/endian/doc/html
>>>
>>> (Sorry but there is a bad link to the reference)
>>>
>>> This is not complex
>>
>> Complexity is in the eye of the beholder. To me, that approach was
>> markedly
>> more complex and confusing.
>>
> What do you find complex in separating the classes endian_pack and endian?

That particular approached seemed to markedly increase surface area.
It might be a good thing to provide the basic endian buffer facility
in addition to the full endian integer facility, but not at the cost
of doubling the surface area.

A policy, as Tomas mentioned, would be one way to do that. I'm about
to reply to him asking for his reaction to an inheritance based
approach.

--Beman

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Endian] Review

Beman Dawes
In reply to this post by Tomas Puverle
All,

On Thu, Sep 8, 2011 at 1:18 PM, Tomas Puverle
<[hidden email]> wrote:

> Hi Beman,
>
>>      - What is your evaluation of the design?
>
> The design of the integer types seems reasonable to me, even though I would
> prefer not to have the overloaded operators on them.  I feel that having the
> operators has the potential to break the boundary between program input and
> the program's processing.  What do you I mean by that?  Typically, I try to
> structure my programs as follows:
>
>        1) Read input data
>        2) Verify input invariants, construct internal representation
>        3) Process internal representation
>        4) Output result
>
> The problem with having operators in the endian types is that they encourage
> the user to continue using them past the point 2).
>
> However, as I said, that's my personal preference and last year plenty of
> people wanted to see the operators included.  Perhaps they could be a policy?
>
> endian<..., bool EnableOperators_ > ?

Consider this alternative:

A class template, perhaps named "endian_buffer", that is the same as
the current class endian except it does not provide the operators. The
typedefs provided would have "buf" added to the name (E.G. big32buf_t,
etc.)

A class template, perhaps named "endian_integer", that publicly
inherits from "endian_buffer" and also provides the operators. In
functionality it would thus be identical to the current class
"endian", including the current typedefs.

Functionally of this approach is exactly the same as Tomas'
endian<..., bool EnableOperators_ > suggestion plus the additional
typedefs. But my sense is that it would clarify the separation of
concerns, and would be a bit easier to explain to new users than a
policy.

It seems to me this addresses the concerns of those who want explicit
separate of the boundary between I/O and processing, and that it will
make users more aware of choices in program organization. Do others
agree?

It seems to me that the cost in terms of both surface area and
implementation complexity is relatively small, so I'm not overly
worried about feature bloat. What to others think?

--Beman

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Endian] Review

Vicente Botet
In reply to this post by Beman Dawes
Le 10/09/11 14:05, Beman Dawes a écrit :

> On Fri, Sep 9, 2011 at 3:20 PM, Vicente J. Botet Escriba
> <[hidden email]>  wrote:
>> Le 09/09/11 15:05, Beman Dawes a écrit :
>>> On Fri, Sep 9, 2011 at 2:32 AM, Vicente
>>> Botet<[hidden email]>wrote:
>>>
>>>> Hi,
>>>>
>>>> I have separated the endian class into a endian_pack that is integer
>>>> agnostic and an endian class that behaves as an integer (You could see
>>>> the
>>>> details in the Sandbox under endian_ext).
>>>>
>>>> Reference docs:
>>>>
>>>>
>>>> http://svn.boost.org/svn/boost/sandbox/endian_ext/libs/integer/endian/doc/html/toward_boost_integer_endian/reference.html#toward_boost_integer_endian.reference.integer_endian_pack_hpp
>>>>
>>>>
>>>>
>>>> http://svn.boost.org/svn/boost/sandbox/endian_ext/libs/integer/endian/doc/html
>>>>
>>>> (Sorry but there is a bad link to the reference)
>>>>
>>>> This is not complex
>>> Complexity is in the eye of the beholder. To me, that approach was
>>> markedly
>>> more complex and confusing.
>>>
>> What do you find complex in separating the classes endian_pack and endian?
> That particular approached seemed to markedly increase surface area.
> It might be a good thing to provide the basic endian buffer facility
> in addition to the full endian integer facility, but not at the cost
> of doubling the surface area.

Next follows the definition of the endian class using endian_pack.

    template <typename E,
         typename T,
         std::size_t n_bits=sizeof(T)*8,
         BOOST_SCOPED_ENUM(alignment) A = alignment::aligned
 > class endian
       : cover_operators< endian< E, T, n_bits, A>, T >
     {
       endian_pack<E, T, n_bits, A> pack_;
       public:
         typedef E endian_type;
         typedef T value_type;
         BOOST_STATIC_CONSTEXPR std::size_t width = n_bits;
         BOOST_STATIC_CONSTEXPR BOOST_SCOPED_ENUM(alignment)
alignment_value = A;

#     ifndef BOOST_ENDIAN_NO_CTORS
         endian() BOOST_ENDIAN_DEFAULT_CONSTRUCT

         template <typename T2>
         explicit endian(T2 val)  : pack_(val)   {  }
#     endif
         endian & operator=(T val) {
             pack_=val;
             return *this;
         }
         operator T() const
         {
           return T(pack_);
         }
         const char* data() const  { return pack_.data(); }

     };

I don't think this could be considered to double the surface.

Best,
Vicente


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Endian] Review

Vicente Botet
In reply to this post by Beman Dawes
Le 10/09/11 14:39, Beman Dawes a écrit :

> All,
>
> On Thu, Sep 8, 2011 at 1:18 PM, Tomas Puverle
> <[hidden email]>  wrote:
>> Hi Beman,
>>
>>>       - What is your evaluation of the design?
>> The design of the integer types seems reasonable to me, even though I would
>> prefer not to have the overloaded operators on them.  I feel that having the
>> operators has the potential to break the boundary between program input and
>> the program's processing.  What do you I mean by that?  Typically, I try to
>> structure my programs as follows:
>>
>>         1) Read input data
>>         2) Verify input invariants, construct internal representation
>>         3) Process internal representation
>>         4) Output result
>>
>> The problem with having operators in the endian types is that they encourage
>> the user to continue using them past the point 2).
>>
>> However, as I said, that's my personal preference and last year plenty of
>> people wanted to see the operators included.  Perhaps they could be a policy?
>>
>> endian<..., bool EnableOperators_>  ?
> Consider this alternative:
>
> A class template, perhaps named "endian_buffer", that is the same as
> the current class endian except it does not provide the operators. The
> typedefs provided would have "buf" added to the name (E.G. big32buf_t,
> etc.)
Yes.
> A class template, perhaps named "endian_integer", that publicly
> inherits from "endian_buffer" and also provides the operators. In
> functionality it would thus be identical to the current class
> "endian", including the current typedefs.
Yes.

> Functionally of this approach is exactly the same as Tomas'
> endian<..., bool EnableOperators_>  suggestion plus the additional
> typedefs. But my sense is that it would clarify the separation of
> concerns, and would be a bit easier to explain to new users than a
> policy.
>
> It seems to me this addresses the concerns of those who want explicit
> separate of the boundary between I/O and processing, and that it will
> make users more aware of choices in program organization. Do others
> agree?
I gues you have at least 1 follower :)
>
> It seems to me that the cost in terms of both surface area and
> implementation complexity is relatively small, so I'm not overly
> worried about feature bloat. What to others think?
>
Happy to see that you have finaly adapted my private proposal included
in Boost.Endian.Ext.

Best,
Vicente


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Endian] Review

John Filo
In reply to this post by Beman Dawes
On 09/10/2011 07:39 AM, Beman Dawes wrote:

> Consider this alternative:
>
> A class template, perhaps named "endian_buffer", that is the same as
> the current class endian except it does not provide the operators. The
> typedefs provided would have "buf" added to the name (E.G. big32buf_t,
> etc.)
>
> A class template, perhaps named "endian_integer", that publicly
> inherits from "endian_buffer" and also provides the operators. In
> functionality it would thus be identical to the current class
> "endian", including the current typedefs.
>
> ...
>
> It seems to me this addresses the concerns of those who want explicit
> separate of the boundary between I/O and processing, and that it will
> make users more aware of choices in program organization. Do others
> agree?
+1.  Assuming you support float/double how do you invision them fitting
into the picture?  Do they only get endian_buffer support?  What about
changing endian_integer to the more generic endian_value and providing
just those operators common to all builtin types?  That would mean
removing the bitwise operators.  If desired, endian_integer could
inherit from endian_value and add in those extra operators.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Endian] Review

Tomas Puverle
In reply to this post by Beman Dawes
> A class template, perhaps named "endian_buffer", that is the same as
> the current class endian except it does not provide the operators. The
> typedefs provided would have "buf" added to the name (E.G. big32buf_t,
> etc.)

Beman,

I would be in full support of this approach.  In fact, it's along the lines of
what I was thinking when I suggested this signature for the "swap" function:

template<typename T_>
swapped_data<T_> native_to_big(T_)

Clearly, your endian_buffer<> is very closely related to the above.  However, in
my original email I didn't pursue the concept because I felt I already made too
many suggestions.

But since we're throwing out ideas, here are a few, off the cuff.  I am not
married to this interface, but I think it points out the different things you
may want to do with an endian_buffer:

template<typename T_, bool Aligned_, std::size_t Size_ = sizeof(T_)>
class endian_buffer;

endian_buffer<void, ...> e1;
uint32_t x1 = big_to_native<uint32_t>(e1);

endian_buffer<uint32_t, ...> e2;
uint32_t x2 = big_to_native(e2);

endian_buffer<uint32_t, true> e3;
uint32_t & x3 = reorder_big_to_native(e3);

Cheers,

Tom




_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Endian] Review

Beman Dawes
On Mon, Sep 12, 2011 at 10:47 AM, Tomas Puverle
<[hidden email]> wrote:

>> A class template, perhaps named "endian_buffer", that is the same as
>> the current class endian except it does not provide the operators. The
>> typedefs provided would have "buf" added to the name (E.G. big32buf_t,
>> etc.)
>
> Beman,
>
> I would be in full support of this approach.  In fact, it's along the lines of
> what I was thinking when I suggested this signature for the "swap" function:
>
> template<typename T_>
> swapped_data<T_> native_to_big(T_)
>
> Clearly, your endian_buffer<> is very closely related to the above.  However, in
> my original email I didn't pursue the concept because I felt I already made too
> many suggestions.
>
> But since we're throwing out ideas, here are a few, off the cuff.  I am not
> married to this interface, but I think it points out the different things you
> may want to do with an endian_buffer:
>
> template<typename T_, bool Aligned_, std::size_t Size_ = sizeof(T_)>
> class endian_buffer;
>
> endian_buffer<void, ...> e1;

I'm not following the above - I'm having trouble understanding the
intent of void and how sizeof(void) would work.

> uint32_t x1 = big_to_native<uint32_t>(e1);
>
> endian_buffer<uint32_t, ...> e2;
> uint32_t x2 = big_to_native(e2);
>
> endian_buffer<uint32_t, true> e3;
> uint32_t & x3 = reorder_big_to_native(e3);

Do I understand correctly from your examples above that your
endian_buffer would to not have any associated endianness, but rather
would depend on the user explicitly calling functions to perform
conversions?

What I was thinking of was an endian_buffer where the endianness was
determined at compile time:

  template <BOOST_SCOPED_ENUM(endianness) E, typename T, std::size_t n_bits,
    BOOST_SCOPED_ENUM(alignment) A = alignment::unaligned>
    class endian_buffer;

So getting a value out after a read looks like this:

    big32buf_t v1;
    ... read into v1...
    int32_t x1 = v1;  // implicit conversion

And then update the value and writing it out looks like:

    ++x1;
    v1 = x1;  // implicit conversion
     ... write v1

I can see a possible advantage for endian_buffers without associated
endianness in applications that don't know what endianness they have
to deal with until runtime. Or am I completely misunderstanding what
you are suggesting?

--Beman

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

[endian] Review

Stewart, Robert
In reply to this post by Tomas Puverle
Herewith is my review of Beman's Boost.Endian library.

____________________
____________________
What is your evaluation of the design?

_____
The two-argument overloads of the [un]conditional swapping functions seem awkward at first blush, but are really the best interface for those operations.  Changing to value returning functions opens the door to mistakes like the following:

   uint32_t const in(123);
   int32_t const out(reorder(in));

There might be a warning, but many programmers routinely ignore such.

As is, the compiler will complain about the conditional functions:

   uint32_t const in(123);
   int32_t out;
   native_to_big(in, out); // Error: ambiguous

The unconditional functions do not provide that benefit because they aren't function templates:

   uint32_t const in(123);
   int32_t out;
   reorder(in, out);

_____
An interesting variation that I've used in the past is a set of cast-style function templates that permit byte swapping one type and returning the result as a different type, after verifying that the resulting value is in range of the target type.  Thus,

   uint32_t const in(std::numeric_limits<uint32_t>::max());
   int32_t const out(to_host<int32_t>(in)); // throws

TMP allows streamlining the range checks done, of course.

_____
Another variation I've used is a checked, two-argument function template with each potentially having a different type:

   template <class T, class U>
   void
   to_host(T & _result, U _value);

(I always prefer output parameters to be first because their position is consistent and it leaves open the possibility of defaulted parameters in the general case.  I'd prefer that in Boost.Endian, but not everyone likes that convention.)

There are two possibilities for dealing with the case when T and U are different.  One is that they be required to have the same, supported size and that they are both signed or both unsigned.  The other is to do a runtime check that the swapped value is within the range of the target type.

_____
I like that the aligned integer types have the longer names because they incur padding and are non-portable.

_____
The overhead implied by the comments in the "Comment on naming" section of the Endian Integer Types page is an excellent reason for splitting the endian class template into endian_buffer and endian_integer as previously suggested.  For C++11, the latter can derive from the former.  For C++03, if POD-ness is important, you'll need to duplicate the former's code in the latter, but it's worthwhile.

____________________
____________________
What is your evaluation of the implementation?

There's already been a good deal of discussion on efficiency and reuse during the review, so I won't rehash it.

The 64b reorder(), for example, looks like a lot of code for an inline function.  I haven't compiled it to inspect the resulting assembly to see how much or little results.  My concern is for the possibility of code bloat arising from the desire to keep this a header-only library.  If those were function templates, then the compiler can choose whether to inline or not, despite the definition being in a header.

The implementation of class endian is uncommented, yet non-trivial.  A future maintainer, even Beman, will benefit from some helpful information inserted now.

The implementation of class endian would be simplified, it appears to me, by passing the endianness along to a detail::store() function template and letting it select the big/little endian store logic.

____________________
____________________
What is your evaluation of the documentation?

It's incomplete.  There are no examples of the conversion functions.  There is no tutorial.  There is no guidance on selecting between the alternatives.  There is no comparison with traditional htnol()-style code.

__________
Endian Home
_____
Introduction to endianness

s/modern Apple, Linux, or Windows computer/computer/ (the OS doesn't matter)

s|a[sic] Oracle/Sun Solaris computer| (the OS doesn't matter)

s/CPU's/CPUs/ (plural, not possessive)

_____
Introduction to the Boost.Endian library

The first and last sentences should be merged: "Boost.Endian is a header only library providing facilities for managing integer endianness."

Endian conversions for native integers: The last sentence makes both absolute and relative statements.  The relative statements are, presumably, relative to endian integer types, but that's not made clear.  I suggest rewording as, "This approach is simple and efficient, but when compared to endian integer types, is less flexible regarding size and alignment, can be harder to manage, is more error prone because of the need to apply endian conversions in unrelated logic paths, all of which can lead to logic errors that are difficult to debug."

Endian integer types: The second sentence has a similar problem, and the last sentence could be combined to correspond with the list of the previous paragraph.  Try this variation: "This approach is simple and, while it can be less efficient than endian conversions, it avoids strict alignment requirements allowing opportunities to pack data more tightly."  I'd follow with this variation of the third sentence: "Furthermore, types of length from 1 to 8 bytes are supported, rather than just the usual 1, 2, 4, and 8 byte lengths."

__________
Conversion Functions

This page is odd.  The synopsis does not include links to the description of the functions.  When I first saw the page, I thought there simply was no documentation for using the various functions.  That documentation does appear in the latter part of the page, but it isn't obvious.  There should be information explaining why conditional and unconditional functions are useful and each should refer to its counterpart.  That is, reorder(uint16_t &) should note that if the intent is not to reorder the bytes regardless of the platform, that bit_to_native(uint16_t &), for example, is appropriate.  The unconditional nature or reorder() escaped me until I realized that these functions always swap bytes, unlike the htonl() family, which I'd never before encountered as necessary.  Thus, use cases for unconditional byte swapping are warranted.

__________
Endian Integer Types

The Conversion Functions page links to the description of big and little endian on the Endian Home page, but the Integer Types page does not.

The example shown on the Integer Types page should be recreated using the Conversion Functions to show the difference in usage.  That example should use sizeof(h) rather than sizeof(header) since that is a safer practice.

_____
Limitations

This section, while important, should not appear so soon on the page.

s/compilers do layout memory/compilers lay out memory/

s/C++0x/C++11/

s/it will be possible to specify the default constructor as trivial/the default constructor is marked as trivial/

s/will no longer disqualify/do not disqualify/

s/will no longer be relying/does not rely/

_____
Feature set

s/Big endian | little endian | native endian byte ordering./Big, little, and native endian byte ordering/

s/Signed | unsigned/Signed and unsigned/

s/Unaligned | aligned/Data may be aligned or unaligned/

s/1-8 byte (unaligned) | 2, 4, 8 byte (aligned)/2, 4, and 8 byte aligned types and 1-8 byte unaligned types/

_____
Typedefs

The column order should correspond with the order of information in the typedef names: alignment, sign, endianness, size.

The discussion of the difference between aligned and unaligned types should not be "buried" in the Typedefs section.

_____
Comment on naming

s/these type grows/these types grows/

Why does "the realization creep in" that the endian integer types are lousy arithmetic integers?  Only profiling would reveal that.  Consequently, you should call that out, perhaps in a "Design Details" section along with the aligned/unaligned discussion, thereby explicitly stating rather than leaving to each library user the discovery of the overhead of these types.

_____
Synopsis

The link from "cover_operators" to a header is surprising.

_____
Members

The description of the default constructor fails to indicate whether the value is left uninitialized, which I presume to be the case.

The description of the conversion operator mixes T and value_type.  While those two are the same, one has to examine the class definition to see that.  Either s/T/value_type/ or s/value_type/T/.

_____
FAQ

s/and both speed and/plus both speed and/

s/disk records/records/ (applies to networking, too)

s/Yes, for sure, compared/There is overhead compared/

s/are usually be faster/are usually faster/

s/types POD's?/types POD?/

s/endian integer types not being POD's/of endian types not being POD/

s/data files are portable/data files that are portable/

The "These types are really just byte-holders" entry should not be a FAQ.  That information, along with a discussion of the overhead of the simple syntax, should be part of the "Design Details" section suggested above.

_____
Design considerations for Boost.Endian integers

Only "Design" is underlined in the section heading.

s/memcpyable/memcpy-able/

_____
Experience

s/used very successful/used very successfully/

_____
Compilation

s/This is ensures that , and/This ensures that class endian is POD, even in C++03, and/

s/POD's/POD/

____________________
____________________
What is your evaluation of the potential usefulness of the library?

Highly useful, but could be more so.

____________________
____________________
Did you try to use the library?

No.  Sorry.

____________________
____________________
How much effort did you put into your evaluation?

I spent about an hour reading the documentation plus a lot of time on the mailing list participating in endian discussions.

____________________
____________________
Are you knowledgeable about the problem domain?

Yes.  I've used the old standby functions, htonl(), ntohl(), and friends over many years.  I've created template functions to automatically select the right function based upon the size of the type being swapped, etc.

____________________
____________________
Do you think the library should be accepted as a Boost library?

There are issues that keep me from saying yes at this time.  There are too many suggested variations and ideas under consideration to accept the library in its present state.  However, a mini-review should be sufficient to evaluate the final form, once Beman determines a course of action, and determine whether to accept it or not.

_____
Rob Stewart                           [hidden email]
Software Engineer                     using std::disclaimer;
Dev Tools & Components
Susquehanna International Group, LLP  http://www.sig.com



________________________________

IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [endian] Review

Beman Dawes
On Tue, Sep 13, 2011 at 3:18 PM, Stewart, Robert <[hidden email]> wrote:

> Herewith is my review of Beman's Boost.Endian library.
>
> ____________________
> ____________________
> What is your evaluation of the design?
>
> _____
> The two-argument overloads of the [un]conditional swapping functions seem awkward at first
> blush, but are really the best interface for those operations.  Changing to value returning
> functions opens the door to mistakes like the following:
>
>   uint32_t const in(123);
>   int32_t const out(reorder(in));

Wow, you have a devious mind:-) I had to read that three times before
I saw the problem!

> There might be a warning, but many programmers routinely ignore such.

Documentation can help a bit. But there are enough different concerns
about the conversion interfaces I'm not sure we can address them all.

> As is, the compiler will complain about the conditional functions:
>
>   uint32_t const in(123);
>   int32_t out;
>   native_to_big(in, out); // Error: ambiguous
>
> The unconditional functions do not provide that benefit because they aren't function templates:
>
>   uint32_t const in(123);
>   int32_t out;
>   reorder(in, out);
>
> _____
> An interesting variation that I've used in the past is a set of cast-style function templates that permit byte swapping one type and returning the result as a different type, after verifying that the resulting value is in range of the target type.  Thus,
>
>   uint32_t const in(std::numeric_limits<uint32_t>::max());
>   int32_t const out(to_host<int32_t>(in)); // throws
>
> TMP allows streamlining the range checks done, of course.
>
> _____
> Another variation I've used is a checked, two-argument function template with each potentially having a different type:
>
>   template <class T, class U>
>   void
>   to_host(T & _result, U _value);
>
> (I always prefer output parameters to be first because their position is consistent and it leaves open the possibility of defaulted parameters in the general case.  I'd prefer that in Boost.Endian, but not everyone likes that convention.)
>
> There are two possibilities for dealing with the case when T and U are different.  One is that they be required to have the same, supported size and that they are both signed or both unsigned.  The other is to do a runtime check that the swapped value is within the range of the target type.

It isn't clear to me that the conversion library is the right place to
do range checks. A major point of using endian conversion functions
rather than endian integers is a need to perform arithmetic and
related operations on native types. Doesn't the same apply to range
checks?

An argument to do range checking on the implicit conversions in the
endian integer types might be stronger, but even there that feels more
like something that should go in a separate checked integer library.

> _____
> I like that the aligned integer types have the longer names because they incur padding and are non-portable.
>
> _____
> The overhead implied by the comments in the "Comment on naming" section of the Endian Integer Types page is an excellent reason for splitting the endian class template into endian_buffer and endian_integer as previously suggested.  For C++11, the latter can derive from the former.  For C++03, if POD-ness is important, you'll need to duplicate the former's code in the latter, but it's worthwhile.

I'm not sure C++03 POD-ness is critical. AFAIK, the only impact would
be use in unions. So maybe in C++03, endian_buffers but not
endian_integers would be usable in POD's.

One critical issue is improving the documentation, including fully
functional (and tested) examples that demonstrate programming the
common use cases using (1) conversions only, (2) endian_buffers, and
(3) endian_integers.

> ____________________
> ____________________
> What is your evaluation of the implementation?
>
> There's already been a good deal of discussion on efficiency and reuse during the review, so I won't rehash it.
>
> The 64b reorder(), for example, looks like a lot of code for an inline function.  I haven't compiled it to inspect the resulting assembly to see how much or little results.  My concern is for the possibility of code bloat arising from the desire to keep this a header-only library.  If those were function templates, then the compiler can choose whether to inline or not, despite the definition being in a header.

I'll give it some thought. What I'd really like to develop is a decent
recipe for being able to support either header-only or
compiled-library approaches.

> The implementation of class endian is uncommented, yet non-trivial.  A future maintainer, even Beman, will benefit from some helpful information inserted now.

Point taken.

> The implementation of class endian would be simplified, it appears to me, by passing the endianness along to a detail::store() function template and letting it select the big/little endian store logic.

The same might applied to a load() function template?

There have also been requests to build the implementation on top of
the conversion functions. That implies that the conversion functions
are expanded to cover unaligned cases, etc.

For me, the key thing that needs to come out of this review is a
complete public interfaced. The details of the implementation will get
looked at, of course, but the public interface needs to be cast in
stone.

>
> ____________________
> ____________________
> What is your evaluation of the documentation?
>
> It's incomplete.  There are no examples of the conversion functions.  There is no tutorial.  There is no guidance on selecting between the alternatives.  There is no comparison with traditional htnol()-style code.

Agree. I was planning to do that before the review, but between
evacuating before Irene, storm cleanup afterward, and a week of
repeated power failures it just didn't happen.

>...

I'm skipping the details of spelling and grammatical suggestions. They
are appreciated and will get applied when the docs are updated.

>Thus, use cases for unconditional byte swapping are warranted.

I've used unconditional swapping in a control path that only occurs
when a swap is needed. For example, when doing file conversions the
format of the current system isn't the determining factor. Rather,
whether or not the input file ordering is the same as the ordering
requested for the output file determines if a reorder is needed, and
that isn't known until run time. The conditional reorder functions are
specified in terms of the unconditional functions, too.

> What is your evaluation of the potential usefulness of the library?
>
> Highly useful, but could be more so.
>
> ____________________
> ____________________
> Did you try to use the library?
>
> No.  Sorry.
>
> ____________________
> ____________________
> How much effort did you put into your evaluation?
>
> I spent about an hour reading the documentation plus a lot of time on the mailing list participating in endian discussions.
>
> ____________________
> ____________________
> Are you knowledgeable about the problem domain?
>
> Yes.  I've used the old standby functions, htonl(), ntohl(), and friends over many years.  I've created template functions to automatically select the right function based upon the size of the type being swapped, etc.
>
> ____________________
> ____________________
> Do you think the library should be accepted as a Boost library?
>
> There are issues that keep me from saying yes at this time.  There are too many suggested variations and ideas under consideration to accept the library in its present state.  However, a mini-review should be sufficient to evaluate the final form, once Beman determines a course of action, and determine whether to accept it or not.

Understood. I'm already committed to a min-review.

Thanks for all the detailed comments. Much appreciated!

--Beman

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost