[llvm-commits] [PATCH][Support] Add Endian.h which contains generic code to deal with endian specific data.

[Rafael Espindola]

>>> Endian.h contains:
>>
>> Should SwapByteOrder be in here?
>
> No, because it contains heavily platform specific code, while this
> provides utilities.

Got it.
>>> * endian
>>> This is a templated struct for reading endian specific data directly
>>> from memory. It is typedefed based on the host platform's endianness
>>> (well, not yet, because there's no LLVM_IS_BIG_ENDIAN or such yet).
>>
>> Why do you always ready data unaligned? The aligned case is probably
>> the common one, no?
>
> Because it's faster to do an unaligned read than check for alignment
> first. The unaligned case is very common for COFF, but non existent
> for ELF, which is why ELF uses the aligned variant. *
>
> * I just realized that the aligned version of
> packed_endian_specific_integral still ends up doing a potentially
> unaligned read. While safe, this should be changed to do a direct
> read, but still byteswap.

OK. I didn't know that COFF had many unaligned parts. Just a
clarification then, if the endian template is to be used only with
unaligned data, shouldn't it have a name with 'unaligned' in it? Would
have made it easier to catch the issue with the aligned
packed_endian_specific_integral :-) Also, do you need to use "uint8_t
Value[sizeof(value_type)]" if you are forwarding the pointer to the
endian::read_* anyway?

>> Also, is
>> the definition of unaligned_access_helper portable?
>
> It should work on every compiler LLVM works on and has been tested on
> MSVC, clang, and gcc; however, it probably should be partially moved
> to compiler.h.
>
> I tried using the strict C/C++ version with bit-shifts, however, no
> compiler was able to optimize it to just a load on x86. Pack produces
> optimal code on x86 and arm, and I assume others.

That is good for now. If someone tries to use it on a compiler that
cannot support it we add the ifdefs and move it.

>> If not I would be somewhat
>> inclined to have the clients handle it. I am worried that a "operator
>> value_type()" might be a bit too convenient :-). Don't you get cases
>> where it is unintentionally called? The code gets too ugly if you have
>> to use an explicit get method?
>
> I don't see where you wouldn't want it to be called. It should work
> exactly as if you just had a value_type except that you can't assign
> to it.
>
> Adding an explicit get method would work, but I feel it would just
> clutter the code.

Your call. I am just a bit concerned about automatic conversions in general.

I think this is OK then with the endian_impl removed and the aligned
versions fixed to do an aligned load.

Cheers,
-- 
Rafael Ávila de Espíndola