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

[Alex Brachet via Phabricator via libc-commits]

abrachet added inline comments.


================
Comment at: libc/src/memory/constants.h:46-48
+#endif
+#endif
+#endif
----------------
Maybe comment the original `#if`'s, 3 `#endif`'s gets a little hard to follow.


================
Comment at: libc/src/memory/utils.h:14-15
+
+#include <stddef.h>
+#include <stdint.h>
+
----------------
We'll need to wait for @sivachandra for this one, because I really don't know. But look how mman.h gets size_t, https://github.com/llvm/llvm-project/blob/master/libc/config/linux/api.td#L7 
https://github.com/llvm/llvm-project/blob/master/libc/config/linux/api.td#L106
I don't think this internal header needs to go through the same process but we don't have `stddef` or `stdint`, and I think we stopped including library headers like this.

Also for @sivachandra the definition of `size_t` includes `stddef` is that ok?


================
Comment at: libc/src/memory/utils.h:20
+// Making sure these functions are inlined by using a private namespace
+namespace {
+
----------------
gchatelet wrote:
> Do we prefer to annotate all functions `static` instead?
The coding standard says so, yes.


================
Comment at: libc/src/memory/utils.h:57
+// Returns the offset from `ptr` to the next cache line.
+inline intptr_t offset_to_next_cache_line(const void *ptr) {
+  return offset_to_next_aligned<LLVM_LIBC_CACHELINE_SIZE>(ptr);
----------------
Is `inline` necessary?


================
Comment at: libc/test/src/memory/utils_test.cpp:12
+
+#include <array>
+
----------------
gchatelet wrote:
> Is this ok to rely on the libc++ for the tests?
No. In any case the burden to use `int [N]` instead of `std::array<int, N>` is very small.


================
Comment at: libc/test/src/memory/utils_test.cpp:24
+  });
+  for (size_t i = 0; i < kExpectedValues.size(); ++i) {
+    ASSERT_EQ((int)is_power2_or_zero(i), kExpectedValues[i]);
----------------
No need for the brackets.

Not that it matters here but I did end up finding the mailing list archive where they talked about preferring signed types for loops if you're curious. http://lists.llvm.org/pipermail/llvm-dev/2019-June/132890.html


================
Comment at: libc/test/src/memory/utils_test.cpp:82
+TEST(UtilsTest, OffsetToNextAligned) {
+  const auto forge = [](size_t offset) -> const void * {
+    return reinterpret_cast<const void *>(offset);
----------------
No need to specify the return, it doesn't really even help readability really when the return type could not possibly be clearer in this case.


================
Comment at: libc/test/src/memory/utils_test.cpp:101-102
+  ASSERT_EQ(offset_to_next_cache_line(forge(LLVM_LIBC_CACHELINE_SIZE - 1)), 1L);
+}
+} // namespace __llvm_libc
----------------
Should there be a space here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73472