[libc-commits] [PATCH] D84875: [libc] Adds strrchr implementation.

[Alex Brachet via Phabricator via libc-commits]

abrachet added a comment.

Clearly I'm too late but there are some problems with this revision.



================
Comment at: libc/src/string/strrchr.cpp:24
+      last_occurrence = str;
+  } while (*str++);
+  return reinterpret_cast<char *>(last_occurrence);
----------------
cgyurgyik wrote:
> sivachandra wrote:
> > Nit: To avoid the post-increment operator, may be:
> > 
> > ```
> > for (; ; ++str) {
> >   if (*str == ch)
> >     last_occurance = str;
> >   if (*str == '\0')
> >     break;
> > }
> > ```
> > 
> > To me personally, mixing post/pre-increment operators with other operators make it hard to read so this comment is also driven by my personal preference.
> I battled with this for a bit as well, but found using a `do...while` loop cleaner. For this reason, I'm going to leave it as is. 
+1 to Siva's suggestion. The break can be `return last_occurance;`


================
Comment at: libc/src/string/strrchr.cpp:16-17
+char *LLVM_LIBC_ENTRYPOINT(strrchr)(const char *src, int c) {
+  unsigned char *str =
+      const_cast<unsigned char *>(reinterpret_cast<const unsigned char *>(src));
+  const unsigned char ch = c;
----------------
Do the const cast at the end. Siva is right too `*str++` is an operator precedence rule away from modifying `src` which is particularly scary when you've const casted early.


================
Comment at: libc/src/string/strrchr.cpp:18
+      const_cast<unsigned char *>(reinterpret_cast<const unsigned char *>(src));
+  const unsigned char ch = c;
+
----------------
`const` not useful here. In any case you can just do `c = static_cast<unsigned char>(c)`. Also why unsigned? Whats wrong with keeping str as a `const char *`?


================
Comment at: libc/test/src/string/strrchr_test.cpp:18-19
+  ASSERT_STREQ(__llvm_libc::strrchr(src, 'a'), "abcde");
+  // Source string should not change.
+  ASSERT_STREQ(src, src_copy);
+}
----------------
This doesn't do anything. You want `ASSERT_STREQ(src, "abcde");` And for the next 3 tests. Also all 4 of these can be consolidated into the same function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84875