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

[Alex Brachet via Phabricator via libc-commits]

abrachet added a comment.

I think there are a few files left over which should have been removed in lieu of their string/details/ counterparts.



================
Comment at: libc/src/CMakeLists.txt:4
 add_subdirectory(string)
+add_subdirectory(memory)
 # TODO: Add this target conditional to the target OS.
----------------
gchatelet wrote:
> sivachandra wrote:
> > Sorry, I did not catch it in the first pass: I think the memory functions are declared in `string.h`. In which case, you should not be adding a new directory called `memory`, but make use of the existing `string` directory.
> I expect quite a few files in here, how about a `details` subfolder?
+1


================
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]);
+  }
----------------
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.


================
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
----------------
Nit: we don't need `({...}) it can just be `{...}`. But I won't it if you prefer how it is currently.


================
Comment at: libc/test/src/string/details/utils_test.cpp:79-81
+  const auto forge = [](size_t offset) {
+    return reinterpret_cast<const void *>(offset);
+  };
----------------
`forge` could be in global scope? Also, is `const` meaningful here?


================
Comment at: libc/test/src/string/details/utils_test.cpp:100-101
+            I(1));
+}
+} // namespace __llvm_libc
----------------
newline between these


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73472