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

[Chris Gyurgyik via Phabricator via libc-commits]

cgyurgyik added inline comments.


================
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;
----------------
abrachet wrote:
> 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.
`*str++` occurs in the order one might expect if they read it left to right, and follows C operator precedence as expected in this implementation. However, I am not tied to this, and understand your concerns. I will change it in a follow-up revision.


================
Comment at: libc/src/string/strrchr.cpp:18
+      const_cast<unsigned char *>(reinterpret_cast<const unsigned char *>(src));
+  const unsigned char ch = c;
+
----------------
abrachet wrote:
> `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 *`?
Can you explain what you mean by `const` not useful here?

Is there a difference between what I have and `c = static_cast<unsigned char>(c)`?

As for your third question, I chose to reinterpret it after reading "each character interpreted as unsigned char": https://en.cppreference.com/w/c/string/byte/strchr


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84875