[libc-commits] [PATCH] D129808: [libc] add unsafe mode to strlen

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/CMakeLists.txt:36
 
+option(LIBC_FAST_UNSAFE_WORD_READ "Use the faster strlen algorithm that depends on undefined behavior, and may fail on some systems." OFF)
+if(LIBC_FAST_UNSAFE_WORD_READ)
----------------
Move the listing of such tuning parameters to a separate file, say `common_tuners.cmake` and include it here. When we have tuners specific to certain functions, for example printf related tuners, they will be listed next to the printf implementation as they will not affect `LIBC_COMPILER_OPTIONS_DEFAULT`.


================
Comment at: libc/src/string/string_utils.h:61
+  for (; reinterpret_cast<uintptr_t>(char_ptr) % sizeof(T) != 0; ++char_ptr) {
+    if (*char_ptr == 0)
+      return char_ptr - src;
----------------
Nit: compare with `\0`.


================
Comment at: libc/src/string/string_utils.h:94
+
+template <typename T>
+static inline void *find_first_character_unsafe(const unsigned char *src,
----------------
Nit: Using the typename `Word` instead of `T` improves the readability.


================
Comment at: libc/src/string/string_utils.h:116
+
+  // Step 3: find the zero in the block
+  for (; *char_ptr != ch && cur < n; ++char_ptr, ++cur) {
----------------
"find the match in the block" ?


================
Comment at: libc/src/string/string_utils.h:141
+  // the gains from reading larger chunks.
+  // The number 4 is a guess, but it seems reasonable.
+  if (n > (sizeof(uintptr_t) * 4)) {
----------------
`ch_mask` is a local var of another function. Can you rephrase this comment so that it will not go stale when the other function changes?


================
Comment at: libc/src/string/string_utils.h:143
+  if (n > (sizeof(uintptr_t) * 4)) {
+    // TODO: pick size of var based on requested size.
+    return find_first_character_unsafe<uintptr_t>(src, ch, n);
----------------
Same as above - not sure what this comment is talking about.


================
Comment at: libc/src/string/string_utils.h:87
+#ifdef LIBC_FAST_UNSAFE_WORD_READ
+  // TODO: pick size of var based on requested size.
+  return string_length_unsafe<uintptr_t>(src);
----------------
nickdesaulniers wrote:
> What requested size? I don't follow what this comment means. Can you expand on it, inline?
This comment is not addressed yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129808