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

[Chris Gyurgyik via Phabricator via libc-commits]

cgyurgyik 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);
----------------
sivachandra wrote:
> Whats the benefit of doing this check? Wouldn't the return statement already handle this case?
It would, but using strlen requires only a single comparison with `0` while the loop below requires a comparison with `0` and a comparison with `ch`.


================
Comment at: libc/src/string/strchr.cpp:22
+
+  for (; *str && *str != c; ++str)
+    ;
----------------
sivachandra wrote:
> 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.
Fixed memchr and added a test as well.


================
Comment at: libc/src/string/strchr.cpp:23
+  for (; *str && *str != c; ++str)
+    ;
+  return *str == c ? str : nullptr;
----------------
PaulkaToast wrote:
> nit: you could put `continue` here but not strictly necessary (:
I will leave it like this for consistency, and I find it slightly more elegant without the `continue` statement. If you are strongly opposed I can change it.


================
Comment at: libc/test/src/string/strchr_test.cpp:37
+
+TEST(StrChrTest, TheSourceShouldNotChange) {
+  const char *src = "abcde";
----------------
sivachandra wrote:
> Instead of a separate test set, test this in all of the above tests?
Is there a benefit to doing this? I want to keep different behavioral components as separate as possible, so that if someone breaks behavior X, only unit test for behavior X breaks, rather than all tests. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83353