[libc-commits] [PATCH] D106901: [libc] add strncmp to strings

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/string/strncmp.cpp:19
+                   (const char *left, const char *right, size_t n)) {
+  if (n < 1) {
+    return 0;
----------------
Nitty comment: Why can it not be just `n == 0`?


================
Comment at: libc/src/string/strncmp.cpp:22
+  }
+  for (size_t i = 1; *left && *left == *right && i < n; ++left, ++right, ++i)
+    ;
----------------
Another nitty comment - This `for` statement is hard to read. Can we make it simpler like this:

```
for (; n > 0; --n, ++left, ++right) {
  char lc = *left;
  if (lc == '\0' || lc != *right)
    break;
}
```


================
Comment at: libc/src/string/strncmp.cpp:24
+    ;
+  return *reinterpret_cast<const unsigned char *>(left) -
+         *reinterpret_cast<const unsigned char *>(right);
----------------
Just pointing out as I am not sure what is right here: casting to `unsigned char` can remove the potential sign and lead to -1 > 0. Even `strcmp` is set up this way so may be clean up both of them in a follow up patch.


================
Comment at: libc/test/src/string/strncmp_test.cpp:159
+}
\ No newline at end of file

----------------
Fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106901