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

Thu Mar 17 16:55:04 PDT 2022

var-const added inline comments.

================
Comment at: libcxx/include/locale:3224-3226
+    for (char __p : __pat.field)
     {
+        switch (__p)
----------------
Mordante wrote:
> 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.
Drive-by comment: I did a quick check using a simple regex `for \([^:]*$` (basically, strings that match `for (` but don't contain the `:` character) and it didn't take long to find a case where it seems like `clang-tidy` should recommend using a range-based for loop. See [[ https://github.com/llvm/llvm-project/blob/e7749d4713a5ec886011ceb0fc821c6723061724/libcxx/include/__random/piecewise_linear_distribution.h#L323-L326 | include/__random/piecewise_linear_distribution.h ]]:
```
    size_t __n = __x.__p_.__b_.size();
    // ...
    for (size_t __i = 0; __i < __n; ++__i)
        __os << __sp << __x.__p_.__b_[__i];
```
If I'm reading this correctly, `__b_` is a vector and it seems like a straightforward case for turning this into a range-based for loop, unless I'm missing something. So I'm a little concerned about why `clang-tidy` doesn't flag this.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121216