[libc-commits] [PATCH] D100571: [libc] Add endianness support

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

In D100571#2691821 <https://reviews.llvm.org/D100571#2691821>, @gchatelet wrote:

> Let me know if you want this header to live somewhere else, or if you had other plans in mind.

The endianess support will rarely ever be updated. So, putting it in common is OK. If it were something which gets updated frequently, I would have suggested a separate library to avoid recompiles of parts which don't use endianess support.



================
Comment at: libc/src/__support/endian.h:12
+
+#include <cstdint>
+
----------------
Include `stdint.h` as `cstdint` is a C++ header.


================
Comment at: libc/src/__support/endian.h:30
+#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+#define LLVM_LIBC_IS_LITTLE_ENDIAN 1
+#else
----------------
gchatelet wrote:
> Here I decided to always define the symbol and set it to 0 or 1 but maybe it's better to leave it undefined. I fear that someone writes `#ifdef LLVM_LIBC_IS_LITTLE_ENDIAN`. It would always be true.
To avoid macro confusion, may be this:

```
namespace __llvm_libc {
namespace internal {

template <bool IsLittleEndian>
class Endian;

template <>
class Endian<__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__> {
 public:
  static uint8_t toBigEndian(uint8_t v) { return v; }
  static uint16_t toBigEndian(uint16_t v) { return __builtin_bswap16(v); }
  ...
};

template <>
class Endian<__BYTE_ORDER__ != __ORDER_LITTLE_ENDIAN__> {
 public:
  static uint8_t toBigEndian(uint8_t v) { return v; }
  static uint16_t toBigEndian(uint16_t v) { return v; }
  ...
};

} // namespace internal

using Endian = internal::Endian<true>;
} // namespace __llvm_libc
```

You can also add `toLittleEndian` methods for the sake of completeness. Also, we probably need tests for this? For, we need the return type, the arg type, and the builtin function name to match. So, tests might protect us from mismatches.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100571/new/

https://reviews.llvm.org/D100571