[libc++] Enable modernize-loop-convert

[• Quuxplusone]

libcxx/include/locale

3224–3226

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.

[philnik]

libcxx/include/locale

3224–3226

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 chars.
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.

[Mordante]

LGTM!

libcxx/include/locale

3224–3226

• 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.

[var-const]

libcxx/include/locale

3224–3226

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 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.

[philnik]

libcxx/include/locale

3224–3226

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.

[var-const]

libcxx/include/locale

3224–3226

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.

[var-const]

libcxx/include/locale

3224–3226

Hmm, what about this one?

for (size_t __i = 0; __i < __densities_.size(); ++__i)
    __densities_[__i] /= __total_area;

This seems self-contained?

[philnik]

libcxx/include/locale

3224–3226

In this case I don't have a good answer. There are a lot of indirections until vector::begin returns a __wrap_iter<_Tp*>. IIUC it could technically be the case that vector::begin SFINAEs away with a non-default allocator. Do you think it would be worth filing a bug against clang-tidy for that?

[var-const]

libcxx/include/locale

3224–3226

If it's not too hard, then yes, it would be awesome to file a bug. Maybe there's some limitation that I'm not aware of, in which case I'd like to know about it, but as a user, I would definitely expect vector to be well-supported.

[ldionne]

I'm fine with this change for the sole reason that I find the diff after easier to read, too.

I'm fine with enabling the clang-tidy check as a "fly-by", however as @Quuxplusone said, if clang-tidy ever starts complaining about stuff and suggesting changes that we don't think are improvements, we'll want to disable those checks. In other words, I'm fine with this change since I think it *is* an improvement, but I want to make it clear that we're not blindly following what the tool tells us.

I agree with others that clang-tidy is probably missing a bunch of places where this can be applied, however we can tackle them as they come, and I guess I find it reassuring that clang-tidy is on the more conservative side.

libcxx/.clang-tidy

 InheritParentConfig: true
 Checks: >
   bugprone-copy-constructor-init,
   bugprone-dangling-handle,
   bugprone-infinite-loop,
 
   -llvm-else-after-return,
   -llvm-header-guard,
   -llvm-qualified-auto,
 
   -misc-unconventional-assign-operator,
 
+  modernize-loop-convert,
   modernize-redundant-void-arg,
 
   readability-duplicate-include,
   readability-function-cognitive-complexity,
   readability-function-size,
   -readability-identifier-naming,
   readability-misplaced-array-index,
   readability-redundant-control-flow,
   readability-redundant-function-ptr-dereference,
   readability-redundant-preprocessor,
   readability-simplify-subscript-expr,
   readability-uniqueptr-delete-release,
 
 CheckOptions:
   - key:   readability-function-cognitive-complexity.Threshold
     value: 143 # TODO: bring that number down
   - key:   readability-function-size.LineThreshold
     value: 194 # TODO: bring that number down
 
 # TODO: investigate these checks
 # bugprone-branch-clone,
 # bugprone-macro-parentheses,
 # cppcoreguidelines-prefer-member-initializer,
-# modernize-loop-convert,
 # modernize-use-bool-literals,
 # modernize-use-default-member-init,
 # modernize-use-equals-default,
 # modernize-use-equals-delete,
 # modernize-use-nullptr,
 # modernize-use-override,
 # portability-restrict-system-includes,
 # readability-function-cognitive-complexity,
 # readability-implicit-bool-conversion,
 # readability-isolate-declaration,
 # readability-redundant-access-specifiers,
 # readability-redundant-declaration,
 # readability-redundant-member-init,
 # readability-simplify-boolean-expr,

libcxx/include/locale

__money_put<_CharT>::__format(char_type* __mb, char_type*& __mi, char_type*& __me,
                               const char_type* __db, const char_type* __de,
                               const ctype<char_type>& __ct, bool __neg,
                               const money_base::pattern& __pat, char_type __dp,
                               char_type __ts, const string& __grp,
                               const string_type& __sym, const string_type& __sn,
                               int __fd)
 {
     __me = __mb;
-    for (unsigned __p = 0; __p < 4; ++__p)
+    for (char __p : __pat.field)
     {
-        switch (__pat.field[__p])
+        switch (__p)
         {
         case money_base::none:
             __mi = __me;
             break;
         case money_base::space:
             __mi = __me;
             *__me++ = __ct.widen(' ');
             break;