Page MenuHomePhabricator

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

Authored by ccotter on Dec 29 2022, 7:45 AM.

Details

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.

Diff Detail

Event Timeline

ccotter created this revision.Dec 29 2022, 7:45 AM
Herald added a project: Restricted Project. · View Herald Transcript
ccotter requested review of this revision.Dec 29 2022, 7:45 AM

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

ccotter updated this revision to Diff 485634.Dec 29 2022, 12:28 PM

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

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?

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 updated this revision to Diff 485643.Dec 29 2022, 2:05 PM

Update docs

ccotter updated this revision to Diff 485655.Dec 29 2022, 6:28 PM

Update tests

ccotter updated this revision to Diff 485656.Dec 29 2022, 6:28 PM

oops, redo diff with correct arc diff command

ccotter updated this revision to Diff 485665.Dec 29 2022, 7:43 PM
  • Remove unused variable
ccotter updated this revision to Diff 487794.Jan 10 2023, 7:00 AM
  • [clang-tidy] Support begin/end free functions in modernize-loop-convert
  • Add release note
  • Update docs
  • Fix tests
  • Remove unused variable
Eugene.Zelenko added inline comments.Jan 10 2023, 7:44 AM
clang-tools-extra/docs/ReleaseNotes.rst
186

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

ccotter marked an inline comment as done.Jan 10 2023, 8:03 AM
ccotter added inline comments.
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 updated this revision to Diff 490028.Jan 17 2023, 9:17 PM
ccotter marked an inline comment as done.
  • fix merge

Friendly ping.

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

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 updated this revision to Diff 500598.Sun, Feb 26, 10:02 AM
ccotter marked 2 inline comments as done.
  • Address feedback
clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
340

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

ccotter updated this revision to Diff 500601.Sun, Feb 26, 10:07 AM
  • fix bad merge in ReleaseNotes
ccotter updated this revision to Diff 500642.Sun, Feb 26, 6:15 PM
ccotter marked an inline comment as done.
  • Add negative test case
  • Fix logic to only consider std:: or ADL calls
ccotter marked 3 inline comments as done.Sun, Feb 26, 6:19 PM
ccotter added inline comments.
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 marked 2 inline comments as done.Sun, Feb 26, 6:27 PM

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 updated this revision to Diff 500647.Sun, Feb 26, 6:39 PM
  • Add comment regarding std::begin/end
ccotter added inline comments.Sun, Feb 26, 6:43 PM
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 added inline comments.Mon, Feb 27, 12:18 PM
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.

consider adding testcase like this:

for (auto I = Obj.begin(5), E = obj.end(5); I != E; ++I) {
  ...
}
ccotter updated this revision to Diff 501734.Wed, Mar 1, 7:07 PM
  • Add another test case