[libc-commits] [PATCH] D73530: [libc] Add a library of standalone C++ utilities.

[Guillaume Chatelet via Phabricator via libc-commits]

gchatelet added a comment.

Thx @sivachandra



================
Comment at: libc/cmake/modules/LLVMLibCRules.cmake:379
+
+function(add_header_library target_name)
+  cmake_parse_arguments(
----------------
Thx!


================
Comment at: libc/utils/CPP/Array.h:17
+  T Data[N];
+  size_t Length = N;
+
----------------
You can use `N` everywhere, you don't need `Length`


================
Comment at: libc/utils/CPP/Array.h:23-29
+  Array() = default;
+
+  Array(const T (&Arr)[N]) {
+    for (size_t i = 0; i < N; ++i) {
+      Data[i] = Arr[i];
+    }
+  }
----------------
abrachet wrote:
> I know you aren't looking for comments, but just a quick one. std::array has no private members or constructors which makes it an aggregate, which gives it the super nice constexpr brace initializer syntax. This constructor would anyway need a reference to a preexisting array and couldn't be initialized like `Array<...> a({1, 2, 3, 4});` If you make `Data` public (and remove `Length` because we can just use `N`) and remove these 2 constructors then it can be initialized like `Array<int, 5> arr {1, 2, 3, 4, 5};`
+1


================
Comment at: libc/utils/CPP/ArrayRef.h:60
+#ifdef __size_t__
+  using size_t = __size_t__;
+#else
----------------
That would be nice to have `size_t` available separately.


================
Comment at: libc/utils/CPP/ArrayRef.h:80
+
+  T *data() const { return const_cast<T *>(ArrayRef<T>::data()); }
+
----------------
[nit] It might be easier to start from `MutableArrayRef` as a base type and inherit from it to constrain constness?


================
Comment at: libc/utils/UnitTest/Test.h:68
+      typename ValType,
+      cpp::EnableIfType<cpp::IsIntegralNotBool<ValType>::Value, ValType> = 0>
   static bool test(RunContext &Ctx, TestCondition Cond, ValType LHS,
----------------
Can we have an `explicit` specialization that handles comparing two `bool`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73530