[libc-commits] [PATCH] D83353: [libc] Add strchr implementation.

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/string/strchr.cpp:19
+  char *str = const_cast<char *>(src);
+  if (!c)
+    return str + __llvm_libc::strlen(str);
----------------
Whats the benefit of doing this check? Wouldn't the return statement already handle this case?


================
Comment at: libc/src/string/strchr.cpp:22
+
+  for (; *str && *str != c; ++str)
+    ;
----------------
To prevent the behavior of this function to depend on the signedness of the `char` type, unsigned values should be compared. It should be exactly like in the memchr implementation. But, I just went looking and it looks like you removed casting `c` to `unsigned char` over there. Left a note about this over there as well.


================
Comment at: libc/test/src/string/strchr_test.cpp:37
+
+TEST(StrChrTest, TheSourceShouldNotChange) {
+  const char *src = "abcde";
----------------
Instead of a separate test set, test this in all of the above tests?


================
Comment at: libc/test/src/string/strchr_test.cpp:62
+
+TEST(StrChrTest, SingleRepeatedCharacterShouldReturnFirstInstance) {
+  const char *dups = "XXXXX";
----------------
You should probably put the test here under `ShouldFindFirstOfDuplicates` above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83353