Details
- Reviewers
sivachandra - Group Reviewers
Restricted Project
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
About strchr, it is the way it is because the standard says, "each character interpreted as unsigned char". I see almost no difference before and after wrt codegen.
Before: https://godbolt.org/z/89sqGrYY9
After: https://godbolt.org/z/4h9eWzKKT
The changes comparing with `\0' are good though.
You are confusing strchr and memchr it seems. Str - standard says char. Memchr - standard says unsigned char.
Easy way to remember this is by knowing: strings are char arrays. Mem is by the raw byte.
To get this reviewed, you should split this up into multiple small and focused patches. I have also left a few comments inline you can use as a general guidelines for splitting.
I think the original author looked at cppreference.com which has almost the same text for strchr and memchr.
strchr - https://en.cppreference.com/w/c/string/byte/strchr
memchr - https://en.cppreference.com/w/c/string/byte/memchr
But, I also looked up the actual standard document and it indeed follows what you are saying.
| libc/src/string/memchr.cpp | ||
|---|---|---|
| 21 ↗ | (On Diff #361325) | Using these explicit casts is probably a good thing, but it mostly cosmetic. If there is a reason beyond just the cosmetics, elaborate that. |
| libc/src/string/memrchr.cpp | ||
| 19 ↗ | (On Diff #361325) | Instead of folding existing code into patterns like this (using an increment and decrement operator along with a another operator like the dereference operator), I would suggest doing the other way round cleanup. For example, the existing code is more readable than this new code. |
| libc/src/string/strchr.cpp | ||
| 19 ↗ | (On Diff #361325) | Comparing with an exact value instead of relying on its boolean interpretation is a good change I think. So, please do it in a separate patch just focused on these kind of changes. It makes the intention clear to the reviewers. |
| libc/src/string/strcpy.cpp | ||
| 14 ↗ | (On Diff #361325) | The changes in this file have to be a patch by itself explaining why and what clearly. |
| libc/src/string/string_utils.h | ||
| 21 ↗ | (On Diff #361325) | For me, most of the changes in this file look like cosmetic churn. So, please explain why you want to do these changes. |
| libc/src/string/strchr.cpp | ||
|---|---|---|
| 19 ↗ | (On Diff #361325) | It actually does what the point of this patch is, which is that it coverts to char, not unsigned char, and because \0 is a char literal for 0, it makes sense here |
| libc/src/string/strcpy.cpp | ||
| 14 ↗ | (On Diff #361325) | It’s an exact copy of the one in memcpy except the argument types align with what the functions are supposed to be handling. |
| libc/src/string/string_utils.h | ||
| 21 ↗ | (On Diff #361325) | It’s actually more efficient this way and closer to the other functions. |
| libc/src/string/strcpy.cpp | ||
|---|---|---|
| 14 ↗ | (On Diff #361325) | Addressed |
A large part of this change is what I would would consider as "churn". So, if you can demonstrate why you want this change, it will be easier to review for people like me (for whom it might not be clear as to why one flavor is better than the other). For example, for code this like:
const char ch = static_cast<char>(c); // c is of type int
The implicit conversion rules make it equivalent to
const char ch = c;
Unless you want the code to work under -Wconversion or something similar, the explicit cast is redundant. Speaking for myself (as a reviewer), if you can let us know the intention of your change, I will be able read it with that in my mind.
Don't get me wrong here; I totally appreciate you taking time to do this. In fact, I have already commented earlier that some of the changes you included in here are good from consistency and readability point of view, but that they should be done separately in order not to mix-up concerns.
| libc/src/string/memrchr.cpp | ||
|---|---|---|
| 19 ↗ | (On Diff #361325) | The code is more efficient this way than the last way. |
One way is to fold this entire patch "Under standardize and optimize" but I would rather split this patch to actually see the "standardize" parts and the "optimize" parts separately. In fact, as it stands, this patch is doing cleanups beyond "optimize" and "standardize". So, I would strongly encourage you to separate out all the individual concerns clearly in different patches.
| libc/src/string/memmove.cpp | ||
|---|---|---|
| 1 ↗ | (On Diff #364590) | Some of the changes in this file have to separated out into a patch of their own. |
| libc/src/string/memory_utils/elements.h | ||
| 1 ↗ | (On Diff #364590) | Same here - some of the changes have to be a patch of their own explaining the rationale clearly. You can combine this change with the change to memmove.cpp. |
| libc/src/string/memrchr.cpp | ||
| 19 ↗ | (On Diff #361325) | Can you please demonstrate that at -O2 or higher, mixing an increment/decrement operator with another operator is more efficient? |