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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/utils/CPP/ArrayRef.h:80
+
+  T *data() const { return const_cast<T *>(ArrayRef<T>::data()); }
+
----------------
gchatelet wrote:
> [nit] It might be easier to start from `MutableArrayRef` as a base type and inherit from it to constrain constness?
Thinking about `ArrayRef` and `MutableArrayRef` twists my mind. But, I am only imitating the LLVM arrangement for consistency. Consistency is probably good to avoid surprising ourselves when switching between different pieces of code.


================
Comment at: libc/utils/UnitTest/Test.h:21
 // a TRUE or FALSE condition. That is because, C library funtions do not
 // return, but use integral return values to indicate true or false
 // conditions. Hence, it is more appropriate to use the other comparison
----------------
abrachet wrote:
> Now that I see it again, I think this should say "return bool" or "return _Bool" 
Fixed this and other typos I found.


================
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,
----------------
abrachet wrote:
> gchatelet wrote:
> > Can we have an `explicit` specialization that handles comparing two `bool`?
>  https://reviews.llvm.org/D73472#inline-667032 and line 20 of this file.
I do not want an integral type to be implicitly converted to `bool`. So, what do you think about supporting `[ASSERT|EXPECT]_[TRUE|FALSE]` via functions like this:

```
// For [ASSERT|EXPECT]_TRUE.
template <
      typename ValType,
      cpp::EnableIfType<cpp::IsBoolType<ValType>::Value, ValType> = true>
static bool testTrue(RunContext &Ctx, ValType Val, const char *ValStr,
                                  const char *File, unsigned long Line) {
  return Val;
}

// For [ASSERT|EXPECT]_FALSE.
template <
      typename ValType,
      cpp::EnableIfType<cpp::IsBoolType<ValType>::Value, ValType> = true>
static bool testFalse(RunContext &Ctx, ValType Val, const char *ValStr,
                                  const char *File, unsigned long Line) {
  return !Val;
}
```

Use of `cpp::IsBoolType` ensures that these functions are restricted to boolean values only.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73530