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

Thu Mar 17 17:18:01 PDT 2022

var-const added inline comments.

================
Comment at: libcxx/include/locale:3224-3226
+    for (char __p : __pat.field)
     {
+        switch (__p)
----------------
var-const wrote:
> philnik wrote:
> > var-const wrote:
> > > 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.
> > In this case `__n` is used in other places as well. The line in your ellipse is `__os << __n`. I think clang-tidy only flags cases where there is no other use than indexing into some range. It's usually very conservative with regards to these kinds of usages. These checks are made to automagically modernize your code base, which wouldn't work if it weren't 100% sure that the fix-it applied is correct.
> Interesting, thanks for the explanation. I don't see what issue could arise from keeping the `__n` variable while reducing the number of its usages, but I can see how it could be a limitation of the tool.
Hmm, what about [[ https://github.com/llvm/llvm-project/blob/5f4a334ded90f80ca16ceb2bf784c62806ee23d1/libcxx/include/__random/piecewise_constant_distribution.h#L193-L194 | this one ]]?
```
    for (size_t __i = 0; __i < __densities_.size(); ++__i)
        __densities_[__i] /= __total_area;
```
This seems self-contained?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121216