[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/strtok_r.cpp:16
+
+char *LLVM_LIBC_ENTRYPOINT(strtok_r)(char *src, const char *delimiter_string,
+                                     char **saveptr) {
----------------
abrachet wrote:
> cgyurgyik wrote:
> > cgyurgyik wrote:
> > > cgyurgyik wrote:
> > > > abrachet wrote:
> > > > > sivachandra wrote:
> > > > > > Would it make sense to implement `strtok` and `strtok_r` over a common function which is equivalent in functionality to `strtok_r`?
> > > > > Making the prototype for this function `restrict` but not the function itself doesn't allow for optimizations. `src` and `delimiter_string` should be marked `__restrict` here and I imagine in a couple of other string functions too
> > > > Yeah you're right, I haven't been using `__restrict` at all. I should've probably asked about this at an earlier point, wasn't sure if there was a reason to use it or not. Will fix this here, and other places in future revisions as well.
> > > I'm struggling to get a static inline function working for both a static char * and a char ** without running into lifetime issues with the `saveptr`. Was there something you had in mind? Maybe I'm missing something simple here.
> > One way I could do this is to have the utility function return a struct `{char *token, char *strtok_string}`, and then set the `strtok_string` to the respective value, `static char *` for strtok, and `dereferenced char**` for strtok_r. Then we can return the `struct.token`.
> `strtok` can just be
> ```lang=cpp
> char *LLVM_LIBC_ENTRYPOINT(strtok)(char *src, const char *delimiter_string) {
>   static char *strtok_str;
>   return __llvm_libc::strtok_r(src, delimiter_string, &strtok_str);
> }
> ```
> There is no lifetime issue.
Yep... that's the simple solution I avoided for much too long. I was trying to do the reverse. Will fix. Thanks abrachet :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85779