[libc-commits] [PATCH] D82724: [libc] Add memchr implementation.

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/string/memchr.cpp:18
+  const unsigned char *str = reinterpret_cast<const unsigned char *>(src);
+  const unsigned char ch = static_cast<unsigned char>(c);
+  for (; n && *str != ch; --n, ++str)
----------------
Do you need the explicit static cast? Without it, wouldn't the compile time implicit cast do the same thing?


================
Comment at: libc/src/string/memchr.cpp:22
+  if (n)
+    return reinterpret_cast<void *>(const_cast<unsigned char *>(str));
+
----------------
I think you don't need need the cast to `void*`.


================
Comment at: libc/src/string/memchr.cpp:24
+
+  return 0;
+}
----------------
Return `nullptr`?


================
Comment at: libc/test/src/string/memchr_test.cpp:18
+  // Should return 'b', 'c', '\0' even when after null terminator.
+  const char *ret = reinterpret_cast<const char *>(
+      __llvm_libc::memchr(reinterpret_cast<const void *>(src), 'b', size));
----------------
To avoid the noisy casts to `const char *`, may be use a helper function call `call_memchr` here and in other tests.


================
Comment at: libc/test/src/string/memchr_test.cpp:19
+  const char *ret = reinterpret_cast<const char *>(
+      __llvm_libc::memchr(reinterpret_cast<const void *>(src), 'b', size));
+  ASSERT_EQ(ret[0], 'b');
----------------
I don't think you need the casts to `void*` here and in the rest of the tests.


================
Comment at: libc/test/src/string/memchr_test.cpp:22
+  ASSERT_EQ(ret[1], 'c');
+  ASSERT_EQ(ret[2], '\0');
+}
----------------
Why not `ASSERT_STREQ`?


================
Comment at: libc/test/src/string/memchr_test.cpp:32
+  ASSERT_EQ(ret[0], 'b');
+  ASSERT_EQ(ret[1], 'c');
+}
----------------
Ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82724