[libc-commits] [PATCH] D106511: [libc] Add explicit C++ casting to strchr, strrchr, memchr, and memrchr

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

To get this reviewed, you should split this up into multiple small and focused patches. I have also left a few comments inline you can use as a general guidelines for splitting.

In D106511#2900060 <https://reviews.llvm.org/D106511#2900060>, @gAlfonso-bit wrote:

> You are confusing strchr and memchr it seems. Str - standard says char. Memchr - standard says unsigned char.

I think the original author looked at cppreference.com which has almost the same text for `strchr` and `memchr`.

`strchr` - https://en.cppreference.com/w/c/string/byte/strchr
`memchr` - https://en.cppreference.com/w/c/string/byte/memchr

But, I also looked up the actual standard document and it indeed follows what you are saying.



================
Comment at: libc/src/string/memchr.cpp:21
+      reinterpret_cast<const unsigned char *>(src),
+      static_cast<unsigned char>(c), n);
 }
----------------
Using these explicit casts is probably a good thing, but it mostly cosmetic. If there is a reason beyond just the cosmetics, elaborate that.


================
Comment at: libc/src/string/memrchr.cpp:19
   for (; n != 0; --n) {
-    const unsigned char *s = str + n - 1;
-    if (*s == ch)
-      return const_cast<unsigned char *>(s);
+    if (*(--str) == ch)
+      return const_cast<unsigned char *>(str);
----------------
Instead of folding existing code into patterns like this (using an increment and decrement operator along with a another operator like the dereference operator), I would suggest doing the other way round cleanup. For example, the existing code is more readable than this new code.


================
Comment at: libc/src/string/strchr.cpp:19
+  for (; *src != ch; ++src)
+    if (*src == '\0')
+      return nullptr;
----------------
Comparing with an exact value instead of relying on its boolean interpretation is a good change I think. So, please do it in a separate patch just focused on these kind of changes. It makes the intention clear to the reviewers.


================
Comment at: libc/src/string/strcpy.cpp:14
 
 namespace __llvm_libc {
----------------
The changes in this file have to be a patch by itself explaining why and what clearly.


================
Comment at: libc/src/string/string_utils.h:21
 static inline size_t string_length(const char *src) {
-  size_t length;
-  for (length = 0; *src; ++src, ++length)
+  const char *const initial = src;
+  for (; *src != '\0'; ++src)
----------------
For me, most of the changes in this file look like cosmetic churn. So, please explain why you want to do these changes.


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

https://reviews.llvm.org/D106511