[libc-commits] [PATCH] D80010: [libc] Add memset and bzero implementations

[Alex Brachet via Phabricator via libc-commits]

abrachet added inline comments.


================
Comment at: libc/src/string/memory_utils/memset_utils.h:20
+template <size_t kBlockSize> static void SetBlock(char *dst, unsigned value) {
+  __builtin_memset(dst, value, kBlockSize);
+}
----------------
gchatelet wrote:
> Theoretically the compiler is allowed to call `memset`here and end up with a recursive call, practically it doesn't happen, however this should be replaced with a `__builtin_memset_inline` once it's available in clang.
Of course I can't find it anymore, but we had for `memcpy` a CMake rule to see if the file contained any references to `memcpy` I think. Is that being done here? 


================
Comment at: libc/src/string/memory_utils/memset_utils.h:9
+
+#ifndef LLVM_LIBC_SRC_MEMORY_UTILS_MEMSET_UTILS_H
+#define LLVM_LIBC_SRC_MEMORY_UTILS_MEMSET_UTILS_H
----------------
Should this have `STRING` in between `SRC` and `MEMORY`? I think clang-tidy might complain.


================
Comment at: libc/src/string/memory_utils/memset_utils.h:27
+static void SetLastBlock(char *dst, unsigned value, size_t count) {
+  const size_t offset = count - kBlockSize;
+  SetBlock<kBlockSize>(dst + offset, value);
----------------
Strange `const`


================
Comment at: libc/src/string/memory_utils/memset_utils.h:95
+    return SetBlockOverlap<64>(dst, value, count);
+  return SetAlignedBlocks<32>(dst, value, count);
+}
----------------
Why choose 32 and not 64? If 32 is the best choice here then why does the `count <= 128` condition exist?


================
Comment at: libc/src/string/memset.cpp:16-17
+void *LLVM_LIBC_ENTRYPOINT(memset)(void *dst, int value, size_t count) {
+  // The value is passed as an int, but the function fills the block of memory
+  // using the `unsigned char` conversion of this value.
+  GeneralPurposeMemset(reinterpret_cast<char *>(dst),
----------------
I think the cast is explanatory enough without the comment. 


================
Comment at: libc/test/src/string/bzero_test.cpp:1
-//===----------------------- Unittests for memcpy -------------------------===//
+//===----------------------- Unittests for bzero --------------------------===//
 //
----------------
`//===-- Unittests for bzero --...`


================
Comment at: libc/test/src/string/bzero_test.cpp:39
       for (size_t i = 0; i < count; ++i)
-        ASSERT_EQ(buffer[align + i], groundtruth[i]);
+        ASSERT_EQ(buffer[align + i], char(0));
       // Everything after copy is untouched.
----------------
There was recently a patch which changed all uses of functional style casts. I don't know if its in the style guide to not use them or not. But maybe it's better to use `(char) 0` or `'\0'`?


================
Comment at: libc/test/src/string/memcpy_test.cpp:38
+      // Return value is `dst`.
+      ASSERT_EQ(ret, (const void *)dst);
       // Everything before copy is untouched.
----------------
Maybe reinterpret_cast to `const void *const`. Obviously it isn't harmful here because ASSERT_EQ takes a `const T` but there is a sneaky const_cast in this C-style cast 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80010