[libc-commits] [PATCH] D85779: [libc] Add strtok_r implementation.

[Chris Gyurgyik via Phabricator via libc-commits]

cgyurgyik added inline comments.


================
Comment at: libc/src/string/string_utils.h:40
+// terminating character is reached, returns a nullptr.
+inline char *string_token(char *src, const char *delimiter_string,
+                          char **saveptr) {
----------------
sivachandra wrote:
> `static`?
Thanks :). This was me trying to debug some quirky lifetime issues with the not-so-elegant approach I was taking beforehand.


================
Comment at: libc/test/src/string/strtok_r_test.cpp:11
+#include "utils/UnitTest/Test.h"
+
+TEST(StrTokReentrantTest, NoTokenFound) {
----------------
sivachandra wrote:
> While having these differently named tests is fine, I am mildly concerned with the fact that when I first read them, the tests seems incomplete. For example, most tests are making only a single call to `strtok_r`.  The results of subsequent calls to `strtok_r` are not tested. I suggest putting all of this in one test called `Various` with each of the tests you have in their own comment annotated scoped block. For example:
> 
> ```
> Test(StrTokRTest, Various) {
>   { // Empty source string and delimiter string
>     char empty[] = "";
>     char *reserve = nullptr;
>     ASSERT_STREQ(__llvm_libc::strtok_r(empty, "",  &reserve), nullptr);
>     // Another call to ensure that a bad state was not written in to |reserve|.
>     ASSERT_STREQ(__llvm_libc::strtok_r(empty, "",  &reserve), nullptr);
>     ASSERT_STREQ(__llvm_libc::strtok_r(nullptr, "",  &reserve), nullptr);
>   }
>   ...
> }
> ```
> 
> That said, putting everything in one test is my personal preference. You can choose to the keep the different test classes as you have currently, but include follow up calls to `strtok_r` as suggested in the above example.
Thanks. Done. I'm still personally a fan of trying to separate behaviors by test, but I tried to group some of the bigger entities here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85779