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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

Since the scope of this change has widened, please update the commit message accordingly. Also, if you are not using arcanist, add the link to the phabricator review page in the commit message. You will have to do this on this review page as well as in your local commit.



================
Comment at: libc/src/string/strchr.cpp:19
+  char *str = const_cast<char *>(src);
+  if (!c)
+    return str + __llvm_libc::strlen(str);
----------------
cgyurgyik wrote:
> 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`.
I would expect that a much higher percentage of calls to `strchr` would be with a non-zero `c`.  In which case, this seems like an optimization for a very special case. For the higher percentage case, it would be an additional conditional. 


================
Comment at: libc/test/src/string/strchr_test.cpp:37
+
+TEST(StrChrTest, TheSourceShouldNotChange) {
+  const char *src = "abcde";
----------------
cgyurgyik wrote:
> 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?
That the source should not change is required in all cases. Likewise, that the result is correct is required in all cases. So, checking that source does not change separately seems incomplete.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83353