[libc-commits] [PATCH] D119002: [libc] Fix compilation with gcc

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

Thanks for doing this. As you have mentioned in the description, please split this into independent patches.



================
Comment at: libc/src/__support/FPUtil/FPBits.h:107
 
-  explicit operator T() { return val; }
+  T val() const { return llvmlibc_builtin_bit_cast<T>(bits); }
+
----------------
To be consistent with other getters, we might want to call this `get_val`.


================
Comment at: libc/src/__support/FPUtil/x86_64/LongDoubleBits.h:99
             cpp::EnableIfType<cpp::IsSame<long double, XType>::Value, int> = 0>
-  explicit FPBits(XType x) : val(x) {
+  explicit FPBits(XType x) : bits(llvmlibc_builtin_bit_cast<UIntType>(x)) {
     // bits starts uninitialized, and setting it to a long double only
----------------
While here, can we make these constructors `constexpr`?


================
Comment at: libc/src/__support/builtin_bit_cast.h:22
+template <class To, class From>
+static To llvmlibc_builtin_bit_cast(const From &from) {
+  static_assert(sizeof(To) == sizeof(From), "To and From must be of same size");
----------------
Few comments, some of them might just be bikeshed kind:

1. Why should the name have `builtin` in it? For that matter, remove the `llvmlibc_` prefix and put it in a namespace.
2. `constexpr` instead of `static`?
3. This should ideally be in  `__support/CPP/bit.h`?


================
Comment at: libc/src/__support/builtin_memcpy.h:21
+template <unsigned SIZE>
+static void llvmlibc_builtin_memcpy(char *__restrict dst,
+                                    const char *__restrict src) {
----------------
Seems like we can just inline this into `llvmlibc_bit_cast`?


================
Comment at: libc/src/fenv/fesetexceptflag.cpp:30
+  const int excepts_as_int = llvmlibc_builtin_bit_cast<IntType>(*flagp);
+  const int excepts_to_set = excepts_as_int & excepts;
   fputil::clear_except(FE_ALL_EXCEPT);
----------------
Why is this required?


================
Comment at: libc/utils/MPFRWrapper/MPFRUtils.cpp:486
+}
+
 namespace internal {
----------------
Why was this block moved?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119002