[llvm-commits] [llvm] r116247 - in /llvm/trunk: include/llvm/Support/MathExtras.h include/llvm/System/SwapByteOrder.h unittests/CMakeLists.txt unittests/Support/SwapByteOrderTest.cpp

[Michael Spencer]

On Wed, Nov 17, 2010 at 10:25 PM, Chris Lattner <clattner at apple.com> wrote:
>
> On Oct 11, 2010, at 2:56 PM, Michael J. Spencer wrote:
>
>> Author: mspencer
>> Date: Mon Oct 11 16:56:16 2010
>> New Revision: 116247
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=116247&view=rev
>> Log:
>> System: Add SwapByteOrder and update Support/MathExtras.h to use it.
>
> Hi Michael,
>
> Catching up on back patches here:
>
>> +
>> +template<typename value_type>
>> +inline
>> +typename enable_if_c<sizeof(value_type) == 1
>> +                     && std::numeric_limits<value_type>::is_integer,
>> +                     value_type>::type
>> +SwapByteOrder(value_type Value) {
>> +  // No swapping needed.
>> +  return Value;
>> +}
>> +
>> +template<typename value_type>
>> +inline
>> +typename enable_if_c<sizeof(value_type) == 2
>> +                     && std::numeric_limits<value_type>::is_integer,
>> +                     value_type>::type
>> +SwapByteOrder(value_type Value) {
>
> This seems like *massive* overkill.  All the template metaprogramming isn't making the code easier to read and follow, it is making it more verbose and difficult to understand.  Instead of defining SwapByteOrder like this, why not just define overloads for signed/unsigned char/short/int/long/long long  ?
>
> -Chris

These were added due to concerns about which types were accepted. This
seemed like the cleanest and most explicit way to say what the intent
of each function was.

Adding all of those overloads seems more complicated to me than this,
but I'm fine either way. As long as there are no surprises to the
user.

- Michael Spencer