[libcxx-commits] [PATCH] D121216: [libc++] Enable modernize-loop-convert

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/locale:3224-3226
+    for (char __p : __pat.field)
     {
+        switch (__p)
----------------
Quuxplusone wrote:
> IMO we don't want to do this.
> - Subjectively, to me, the old code looks easier to read at a glance. Here I'd have to scroll up to figure out what `__pat.field` is.
> - This is a C++03 codepath, but ranged for loops are C++11. I guess obviously Clang supports them as an extension, and we have a general practice of permitting Clang's extensions in our "C++03" codepaths, so this bullet point should not be a blocker IMO.
> - I'm shocked that this is the //one// place in all of libc++ that the machine thinks would benefit from a ranged for loop. This leads me cynically to suspect that the machine has a lot of false negatives right now. And //that// leads me to worry that like six months from now, clang-tidy is going to get improvements and start flagging all over libc++ and we're going to have to turn off `modernize-loop-convert` because now it's too noisy.
> - And //that// illustrates my repeated point about all this: **If you weren't making this diff just to appease the machine, would it be a good diff in its own right?** Six months from now, when we turn off clang-tidy, are we also going to wish we could revert this one diff? (Obviously //I// think so.) If so, then we shouldn't make this diff now, either. We should use the machine to support libc++ style; we shouldn't contort libc++ style to please the machine.
My first thought when I saw this was: Where does that magic `4` come from?
How do you know now what `__pat.field` is? There is no indication what it is in the old code. With the new code I at least know that `__pat.field` is some sort of container of `char`s.
I'm not really concerned with hypotheticals. It's not a good strategy to base decisions on IMO. Taking it to the extreme, how do you know there will always be switch-cases in C++? The conclusion would be that we shouldn't use them anywhere. That's simply ridiculous.
I don't think I can say anything about you last point, since I think the new version //is// better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121216