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

[Alex Brachet via Phabricator via libc-commits]

abrachet 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;
----------------
cgyurgyik wrote:
> 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.
I'm not questioning the order of precedence, I'm saying for data that we are not allowed to change we should rely on const. I am tied to doing the const_cast at the end. `const_cast` is dangerous and we should only do it when we absolutely need to.


================
Comment at: libc/src/string/strrchr.cpp:18
+      const_cast<unsigned char *>(reinterpret_cast<const unsigned char *>(src));
+  const unsigned char ch = c;
+
----------------
cgyurgyik wrote:
> 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
`const` is useful as a contract for shared state. There isn't a compelling reason to make private state const in my opinion.

Not after optimizations presumably.

FWIW the C standard says "The strrchr function locates the last occurrence of c (converted to a char) in the string pointed to by s" Regardless signedness won't change equality so this is just an extra step we don't need to take as it's just adding unnecessary code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84875