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
On Thu, Sep 8, 2011 at 1:18 PM, Tomas Puverle < Tomas.Puverle@morganstanley.com> 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
AMDG On 09/08/2011 12:23 PM, Beman Dawes wrote:
On Thu, Sep 8, 2011 at 1:18 PM, Tomas Puverle < Tomas.Puverle@morganstanley.com> 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
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 < Tomas.Puverle@morganstanley.com> 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/ht... http://svn.boost.org/svn/boost/sandbox/endian_ext/libs/integer/endian/doc/ht... (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 -- View this message in context: http://boost.2283326.n4.nabble.com/Endian-Review-tp3799423p3800913.html Sent from the Boost - Dev mailing list archive at Nabble.com.
On Fri, Sep 9, 2011 at 2:32 AM, Vicente Botet <vicente.botet@wanadoo.fr>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/ht...
http://svn.boost.org/svn/boost/sandbox/endian_ext/libs/integer/endian/doc/ht...
(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
Le 09/09/11 15:05, Beman Dawes a écrit :
On Fri, Sep 9, 2011 at 2:32 AM, Vicente Botet<vicente.botet@wanadoo.fr>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/ht...
http://svn.boost.org/svn/boost/sandbox/endian_ext/libs/integer/endian/doc/ht...
(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
On Fri, Sep 9, 2011 at 3:20 PM, Vicente J. Botet Escriba <vicente.botet@wanadoo.fr> wrote:
Le 09/09/11 15:05, Beman Dawes a écrit :
On Fri, Sep 9, 2011 at 2:32 AM, Vicente Botet<vicente.botet@wanadoo.fr>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/ht...
http://svn.boost.org/svn/boost/sandbox/endian_ext/libs/integer/endian/doc/ht...
(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
Le 10/09/11 14:05, Beman Dawes a écrit :
On Fri, Sep 9, 2011 at 3:20 PM, Vicente J. Botet Escriba <vicente.botet@wanadoo.fr> wrote:
Le 09/09/11 15:05, Beman Dawes a écrit :
On Fri, Sep 9, 2011 at 2:32 AM, Vicente Botet<vicente.botet@wanadoo.fr>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/ht...
http://svn.boost.org/svn/boost/sandbox/endian_ext/libs/integer/endian/doc/ht...
(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
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
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
All, On Thu, Sep 8, 2011 at 1:18 PM, Tomas Puverle <Tomas.Puverle@morganstanley.com> 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
Le 10/09/11 14:39, Beman Dawes a écrit :
All,
On Thu, Sep 8, 2011 at 1:18 PM, Tomas Puverle <Tomas.Puverle@morganstanley.com> 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
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.
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
On Mon, Sep 12, 2011 at 10:47 AM, Tomas Puverle <Tomas.Puverle@morganstanley.com> 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
participants (6)
-
Beman Dawes -
John Filo -
Steven Watanabe -
Tomas Puverle -
Vicente Botet -
Vicente J. Botet Escriba