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

[Alf via Phabricator via libc-commits]

gAlfonso-bit marked an inline comment as done.
gAlfonso-bit added a comment.

For this PR, think of the code not in terms of what changed, but how the code is.

Str functions deal with chars, mem deals with unsigned chars, and in this case, changing argument types alone isn’t enough, as some auxiliary changes have to be made to accommodate this, which includes changing includes in strcpy to not rely on memcpy, and direct comparisons with a char literal.



================
Comment at: libc/src/string/strchr.cpp:19
+  for (; *src != ch; ++src)
+    if (*src == '\0')
+      return nullptr;
----------------
sivachandra wrote:
> 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.
It actually does what the point of this patch is, which is that it coverts to char, not unsigned char, and because \0 is a char literal for 0, it makes sense here


================
Comment at: libc/src/string/strcpy.cpp:14
 
 namespace __llvm_libc {
----------------
sivachandra wrote:
> The changes in this file have to be a patch by itself explaining why and what clearly.
It’s an exact copy of the one in memcpy except the argument types align with what the functions are supposed to be handling.


================
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)
----------------
sivachandra wrote:
> For me, most of the changes in this file look like cosmetic churn. So, please explain why you want to do these changes.
It’s actually more efficient this way and closer to the other functions.


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

https://reviews.llvm.org/D106511