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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/CMakeLists.txt:4
 add_subdirectory(string)
+add_subdirectory(memory)
 # TODO: Add this target conditional to the target OS.
----------------
abrachet wrote:
> 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
I am OK even without a sub-directory. But, in case you want to add one, name it `memory_utils` may be?


================
Comment at: libc/src/memory/CMakeLists.txt:8
+target_include_directories(memory_utils INTERFACE ${CMAKE_CURRENT_SOURCE_DIR})
+target_link_libraries(memory_utils INTERFACE memory_constants)
----------------
gchatelet wrote:
> sivachandra wrote:
> > Its odd that you have to add link libraries for header only libraries. Is it because this is the only way to setup the dependencies correctly?
> It seems to be the way to declare transitive dependency on header only libraries yes.
> https://cmake.org/pipermail/cmake/2017-October/066366.html
> 
> If someone knows a better way I'm happy to take any suggestions.
You should not need all of this now I think. Just use `add_header_library`.


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



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73472