[llvm-commits] [PATCH][System] Add SwapByteOrder and move implementation from Support/MathExtras.h.

[Michael Spencer]

On Wed, Sep 15, 2010 at 6:50 PM, Chris Lattner <clattner at apple.com> wrote:
>
> On Sep 14, 2010, at 2:31 PM, Michael Spencer wrote:
>
>> On Tue, Sep 14, 2010 at 5:18 PM, Michael Spencer <bigcheesegs at gmail.com> wrote:
>>> The attached patch adds the SwapByteOrder function template to System.
>>> This patch is a
>>> prerequisite for the object file library I am writing.
>>>
>>> I want to replace the functions in MathExtras.h with SwapByteOrder
>>> because it is a generic implementation that makes it easier to write
>>> generic byte order independent code. This patch has no functionality
>>> changes.
>>>
>>> The optimized versions are platform dependent, and thus belong in System anyway.
>>>
>>> - Michael Spencer
>>>
>>
>> It had to happen eventually :(. Now the patch it attached.
>
> Hi Michael,
>
> What's wrong with the existing ByteSwap_XX functions?  Why is a template better?  Can you give an example of how this is better?
>
> Note that you're likely to run into problems on some systems (cygwin?) where uint32_t != unsigned.
>
> -Chris

If you look at the other patch I posted ([PATCH][Support] Add Endian.h
which contains generic code to deal with endian specific data) it is
used to byte swap an arbitrary integral type like so:

template<typename value_type, int host, int target>
static typename enable_if_c<host != target, value_type>::type
SwapByteOrderIfDifferent(value_type value) {
    return sys::SwapByteOrder<value_type>(value);
}

Which is then used to directly read from memory (in this case a memory
mapped file):

template<endianness host_endianness>
struct endian_impl {
  template<typename value_type>
  static value_type read_le(const void *memory) {
    return SwapByteOrderIfDifferent<value_type, host_endianness, little>(
      *reinterpret_cast<const value_type *>(memory));
  }
  // other read and write functions.
};

Which is then used to create structs with "normal" looking members
that can directly reference memory mapped files:

template<typename value_type,
         endianness target_endianness,
         alignment target_alignment>
class packed_endian_specific_integral;

template<typename value_type>
class packed_endian_specific_integral<value_type, little, unaligned> {
public:
  operator value_type() const {
    return endian::read_le<value_type>(Value);
  }
private:
  uint8_t Value[sizeof(value_type)];
};

// Other specializations...

Which then have typedefs like:

typedef packed_endian_specific_integral<uint32_t, little, unaligned>
little_uint32_t;

As you can see, the value_type is defined a long way away from where
the byte swap occurs. SwapByteOrderIfDifferent could be specialized to
deal with the different value_types, but that belongs in System, and
is useful for others.

In short. It reduces the number of explicit template specializations
required, and therefore complexity.

And as for uint32_t != unsigned. Two things:

1) By unsigned you mean the the keyword unsigned, by itself. Like
"unsigned hey_look_im_a_var = 0;" right?
2) The ByteSwap_XX functions already have uintxx_t types, so I don't
see how that affects this code.

Thanks

- Michael Spencer