[clang-tidy] Support begin/end free functions in modernize-loop-convert

[ccotter]

Summary

The modernize-loop-convert check will now match iterator based loops
that call the free functions 'begin'/'end', as well as matching the
free function 'size' on containers.

Test plan: Added unit test cases matching free function calls on
containers, and a single negative test case for length() which is not
supported.

[Eugene.Zelenko]

It'll be good idea to mention change in Release Notes.

[Eugene.Zelenko]

I just look into check documentation, new feature should be described with example there too.

[ccotter]

In D140760#4019680, @Eugene.Zelenko wrote:

I just look into check documentation, new feature should be described with example there too.

Would you mind clarifying where I should add documentation to? In the release notes, or within LoopConvertCheck.cpp itself?

[Eugene.Zelenko]

In D140760#4019708, @ccotter wrote:

In D140760#4019680, @Eugene.Zelenko wrote:

I just look into check documentation, new feature should be described with example there too.

Would you mind clarifying where I should add documentation to? In the release notes, or within LoopConvertCheck.cpp itself?

Sorry, if I was not clear. I meant clang-tools-extra/docs/clang-tidy/checks/modernize/loop-convert.rst.

[ccotter]

Update docs

[ccotter]

Update tests

[ccotter]

oops, redo diff with correct arc diff command

[ccotter]

• Remove unused variable

[ccotter]

• [clang-tidy] Support begin/end free functions in modernize-loop-convert
• Add release note
• Update docs
• Fix tests
• Remove unused variable

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

186

Please keep alphabetical order (for check names) in this section.

[ccotter]

• alpha

[ccotter]

clang-tools-extra/docs/ReleaseNotes.rst

186

Done, and I fixed what looks like an existing out of order check in the release notes: https://reviews.llvm.org/D141391

[ccotter]

• fix merge

[ccotter]

Friendly ping.

[carlosgalvezp]

LGTM, minor comments. I'm not familiar with the implementation so I'm not very confident reviewing it, would be good to get some more expert eyes on it. Tests look solid!

clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp

340

Remove const, it's unused.

346

if (const auto *Member = ...)

[PiotrZSL]

In short, better support like this than lack of suport.
Consider adding option for disabling those free standing functions, reason why I'm telling this is that in case of false-positives, it may be easier just to disable part of functionality than disabling entire check or mass adding NOLINTs.

clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp

173–174

Now it will also catch calls to single parameter methods...
maybe unless(cxxMethodDecl()), after all we want to catch only functions

192

Now it will also catch calls to single parameter methods...
maybe unless(cxxMethodDecl()), after all we want to catch only functions

302

Consider exclude single parameter methods.

920

`else return Nodes.getNodeAs<CallExpr>(EndCallName) != nullptr;`

clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp

449

Also you could do a mix of member begin and free standing end ?
will it be supported or it wont ?

456

would be nice to validate if begin/end got same argument, but for that probably we would need to extract isIdenticalStmt function from clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp into separate matcher, so thats not a topic for this change.

[ccotter]

• Address feedback

clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp

340

good call..this is another check/cppcoreguideline anyway.

[ccotter]

• fix bad merge in ReleaseNotes

[ccotter]

• Add negative test case
• Fix logic to only consider std:: or ADL calls

[ccotter]

clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp

192

Updated this to properly only look at 1) a qualified call to std::begin etc, or 2) an ADL call to begin etc.

920

Is this in the style guide? NotNullTerminatedResultCheck.cpp, SignedCharMisuseCheck.cpp, UnconventionalAssignOperatorCheck.cpp have equivalent conditionals that do not check against nullptr.

clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp

449

No, I updated the logic and added test cases. I don't think mixing makes sense since range based loops look at symmetric member methods or ADL lookups but not mixed cases. This would not be a common case anyway in code I expect.

456

The existing logic handles this. I'll add a negative test case.

[ccotter]

I'm not sure about the false positive case. Range based for loops reduce to calls to ADL lookup begin/end (or member function calls to begin/end). I believe the same "false positive" argument (whatever that may be) would apply in an equivalent manner to the member call case.

[ccotter]

• Add comment regarding std::begin/end

[ccotter]

clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp

142

std::begin etc are singled out single they delegate to the member function begin, although they're really unrelated to ADL begin etc.

[PiotrZSL]

clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp

920

its not about checking for nullptr, its about simplifying if else if else, you dont need to explicitly compare to nullptr, I'm just used to it.
You can easily depend on implicit bool conversion here, its fine.
Simply you got if something return true else return false, you can write just return something.

[PiotrZSL]

consider adding testcase like this:

for (auto I = Obj.begin(5), E = obj.end(5); I != E; ++I) {
  ...
}

[ccotter]

• Add another test case