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

[Guillaume Chatelet via Phabricator via libc-commits]

gchatelet marked 7 inline comments as done.
gchatelet added a comment.

A few questions come with this patch and will help establish guidelines for the next ones.



================
Comment at: libc/cmake/modules/LLVMLibCRules.cmake:320
   foreach(dep IN LISTS LIBC_UNITTEST_DEPENDS)
-    get_target_property(dep_type ${dep} "TARGET_TYPE")
-    if (dep_type)
-      string(COMPARE EQUAL ${dep_type} ${ENTRYPOINT_OBJ_TARGET_TYPE} dep_is_entrypoint)
-      if(dep_is_entrypoint)
-        get_target_property(obj_file ${dep} "OBJECT_FILE_RAW")
-        list(APPEND library_deps ${obj_file})
-        continue()
+    # If the dep is a normal CMake library target then add it to the list of
+    # library_deps.
----------------
I had to test for non entrypoint libraries first as the `INTERFACE` do not have the `"TARGET_TYPE"` property.


================
Comment at: libc/src/memory/constants.h:1
+//===-------------------------- Memory constants --------------------------===//
+//
----------------
I assume these files will stay as implementation details and are not to be exposed but feel free to challenge this assumption.


================
Comment at: libc/src/memory/constants.h:12
+
+// LLVM_LIBC_CACHELINE_SIZE
+//
----------------
This comes from https://github.com/abseil/abseil-cpp/blob/master/absl/base/optimization.h


================
Comment at: libc/src/memory/utils.h:20
+// Making sure these functions are inlined by using a private namespace
+namespace {
+
----------------
Do we prefer to annotate all functions `static` instead?


================
Comment at: libc/test/src/memory/utils_test.cpp:12
+
+#include <array>
+
----------------
Is this ok to rely on the libc++ for the tests?


================
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 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`


================
Comment at: libc/test/src/memory/utils_test.cpp:85
+  };
+  ASSERT_EQ(offset_to_next_aligned<16>(forge(0)), 0L);
+  ASSERT_EQ(offset_to_next_aligned<16>(forge(1)), 15L);
----------------
@sivachandra `ASSERT_EQ` needs to have the same type on both sides so I had to use the `L` suffix literal. Technically we should write `intptr_t(0)` here and below but it hurts readability.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73472