[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:
> 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.
Agreed, `const_cast` later is the safer option. I did the cast early to avoid having to call it in the while loop.


================
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:
> 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.
To me, `const` simply means I don't want the value associated with it to change. In this case, I don't want `ch` to change later in the function call. This is another layer of type safety.



> 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" 

Noted, I'll avoid the unsigned char conversion here.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84875