[libc-commits] [PATCH] D73472: [libc] Add utils for memory functions

[Guillaume Chatelet via Phabricator via libc-commits]

gchatelet added inline comments.


================
Comment at: libc/test/src/memory/utils_test.cpp:25
+  for (size_t i = 0; i < kExpectedValues.size(); ++i) {
+    ASSERT_EQ((int)is_power2_or_zero(i), kExpectedValues[i]);
+  }
----------------
sivachandra wrote:
> abrachet wrote:
> > gchatelet wrote:
> > > sivachandra wrote:
> > > > gchatelet wrote:
> > > > > @sivachandra there are no `ASSERT_TRUE`/`ASSERT_FALSE` and `ASSERT_EQ` do not handle `bool` values so I had to cast `is_power2_ro_zero` to `int`
> > > > The reason why I have chosen not to have `ASSERT_[TRUE|FALSE]`, and not to support `ASSERT_EQ` for `bool` is this: In a C library, a return value is often more than tri-state. Hence, not having support for `bool` forces one to check for the exact return value.
> > > > 
> > > > I can see that it is being an annoyance for cases such as yours. But, can you actually make functions like `is_power2_or_zero` return an `int` value?
> > > Thx for the explanation I understand the rationale now.
> > > 
> > > This makes sense when testing pure `C` code but most of the implementation for the `llvm-libc` will heavily rely on `C++` so it seems like an unnecessary burden.
> > > 
> > > For instance `utils.h` has a templated function so it does not compile as pure `C`.
> > > 
> > > I believe all tests are written in `C++` as well (`LibCUnitTest` is itself `C++`)
> > > 
> > > > I can see that it is being an annoyance for cases such as yours. But, can you actually make functions like is_power2_or_zero return an int value?
> > > 
> > > Then I'd have to think about the tri-state in the implementation as well which is not ideal. `bool` is a great `C++` tool that I want to use.
> > > 
> > > How about the opposite?
> > > We could have macros to disable the `bool` versions when testing the `C` functions themselves?
> > > 
> > > ```
> > > TEST(TestCFunc, strlen) {
> > > START_LIBC_UNIT_TEST_C
> > > /// ASSERT_[TRUE|FALSE] and ASSERT_EQ for bool are disabled
> > > STOP_LIBC_UNIT_TEST_C
> > > }
> > > ```
> > > 
> > > I might be biased but I think that most of the unittests will be devoted to the internals of the library and not the actual lic functions.
> > > 
> > > We could go even simpler with a one liner at the top of the test file `UNIT_TESTS_FOR_C_FUNCTION` to disable the problematic checks. WDTY?
> > Most unit tests I presume would end up being for C library functions which even if written in C++ will not return bool. -1 for `START_LIBC_UNIT_TEST_C` it's just too ugly and verbose. At most we could do something like
> > ```
> > #define LIBC_WANT_BOOL_SPECIALIZATIONS
> > #include "utils/UnitTest/Test.h"
> > ```
> > But really I think this is too much too. Realistically we can provide *_EQ that handles bool because no such type exists in C so no library functions would return it, and as long as we don't have *_TRUE, *_FALSE we still have the safety Siva is hoping to achieve. I don't have a strong preference though.
> Even I do not like the enable/disable marcos. But, what do you think about this which adds `[EXPECT|ASSERT]_[TRUE|FALSE]`: https://reviews.llvm.org/D73668
> 
> In this test, you can use them like:
> 
> ```
> if (kExpectedValues[i] == 1)
>   ASSERT_TRUE(is_power2_or_zero(i));
> else
>   ASSERT_FALSE(is_power2_or_zero(i));
> 
Sounds good, waiting for D73668 to be in then.
Quick question though why do you prefer a `EXPECT_TRUE`/`EXPECT_FALSE` over a `EXPECT_EQ(bool, bool)` with an `explicit` instantiation? `explicit` would guard against automatic promotions to `bool`.


================
Comment at: libc/test/src/string/details/utils_test.cpp:17
+TEST(UtilsTest, IsPowerOfTwoOrZero) {
+  static const std::array<int, 65> kExpectedValues({
+      1, 1, 1, 0, 1, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, // 0-15
----------------
abrachet wrote:
> Nit: we don't need `({...}) it can just be `{...}`. But I won't it if you prefer how it is currently.
Acknowledged. I'm keeping it as is because of the switch to `cpp::ArrayRef`


================
Comment at: libc/test/src/string/details/utils_test.cpp:25
+  for (size_t i = 0; i < kExpectedValues.size(); ++i)
+    ASSERT_EQ((int)is_power2_or_zero(i), kExpectedValues[i]);
+}
----------------
abrachet wrote:
> I think all of the `ASSERT_*`'s could be `EXPECT_*` instead. As a matter of preference I think ASSERT should be used to test invariants that would lead to other failures or crashes. No strong preference.
Yes indeed, my mistake.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73472