[libc-commits] [PATCH] D85615: [libc] Add strtok implementation.

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/string/strtok.cpp:16
+
+char *LLVM_LIBC_ENTRYPOINT(strtok)(char *src, const char *delimiters) {
+  cpp::Bitset<256> bitset;
----------------
Nit: `delimiter_string`


================
Comment at: libc/src/string/strtok.cpp:17
+char *LLVM_LIBC_ENTRYPOINT(strtok)(char *src, const char *delimiters) {
+  cpp::Bitset<256> bitset;
+  for (; *delimiters; ++delimiters)
----------------
Nit: `delimiter_set`


================
Comment at: libc/src/string/strtok.cpp:21
+
+  static char *temp = nullptr;
+  src = src ? src : temp;
----------------
Nit: Can you move this out side of this function and call it `strtok_str`?


================
Comment at: libc/src/string/strtok.cpp:23
+  src = src ? src : temp;
+  // Find first occurrence of a delimiter or null terminator.
+  for (; *src && bitset.test(*src); ++src)
----------------
I think the comments are reversed. It should be "non-delimiter" here and "delimiter" below? But, I think comments are just a distraction here as the code speaks for itself.


================
Comment at: libc/src/string/strtok.cpp:27
+
+  char *delimiter_span = src;
+  // Find first occurrence of a non-delimiter or null terminator.
----------------
Nit: s/`delimiter_span`/`token`


================
Comment at: libc/src/string/strtok.cpp:33
+  if (*temp)
+    *temp++ = '\0';
+  return delimiter_span;
----------------
A readability thing to not mix increment/decrement operators with other operators:

```
    *strtok_str = '\0';
     ++strtok_str;
```


================
Comment at: libc/test/src/string/strtok_test.cpp:61
+  token = __llvm_libc::strtok(nullptr, "_:,_");
+  ASSERT_STREQ(token, nullptr);
+  // Subsequent calls after hitting the end of the string should also return
----------------
Does `strtok` as implemented above ever return `nullptr`?  This got me to go look why `ASSERT_STREQ` is happy. It turns out, our `STREQ` comparison first converts the string to `llvm::StringRef` which makes an empty string compare equal to a nullptr! That needs fixing separately.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85615