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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision as: Mordante.
Mordante added a comment.

LGTM!



================
Comment at: libcxx/include/locale:3224-3226
+    for (char __p : __pat.field)
     {
+        switch (__p)
----------------
philnik wrote:
> 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.
* I'm also surprised this is the only place discovered by clang-tidy. If this indeed is due to clang-tidy not performing optimal I prefer to tackle this when the time comes; either by disabling this check or fix the other occurrences. We're using the latest release of clang-tidy and not ToT so we have control when we roll out a newer version of clang-tidy.
* I find this version more readable. During the review I had to check why the value was `4` and whether the change to the range-based for loop was safe.
* I agree we should be careful with enabling check to make sure it passes C++03, however I trust the CI will tell us when we make a mistake.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121216