This is an archive of the discontinued LLVM Phabricator instance.

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

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
231

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
231

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
318

Remove const, it's unused.

324

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
158–159

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

175

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

283

Consider exclude single parameter methods.

871

`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.Feb 26 2023, 10:02 AM
ccotter marked 2 inline comments as done.
  • Address feedback
clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
318

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

ccotter updated this revision to Diff 500601.Feb 26 2023, 10:07 AM
  • fix bad merge in ReleaseNotes
ccotter updated this revision to Diff 500642.Feb 26 2023, 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.Feb 26 2023, 6:19 PM
ccotter added inline comments.
clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
175

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

871

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.Feb 26 2023, 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.Feb 26 2023, 6:39 PM
  • Add comment regarding std::begin/end
ccotter added inline comments.Feb 26 2023, 6:43 PM
clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
135

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.Feb 27 2023, 12:18 PM
clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
871

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.Mar 1 2023, 7:07 PM
  • Add another test case
ccotter updated this revision to Diff 508474.Mar 26 2023, 9:41 PM
  • fix bad rebase

What about classes that doesn't have begin/end method but got cbegin/cend ?
I thing there is open issue for that.

What about classes that doesn't have begin/end method but got cbegin/cend ?
I thing there is open issue for that.

Should we handle that in a separate patch?

Should we handle that in a separate patch?

Ok.

LegalizeAdulthood resigned from this revision.Mar 28 2023, 1:22 PM
ccotter updated this revision to Diff 509874.Mar 30 2023, 6:35 PM
ccotter marked 3 inline comments as done.
  • format
clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
158–159

I think usesADL() accomplishes the same thing here.

bump - any other feedback?

PiotrZSL added inline comments.Apr 9 2023, 9:58 AM
clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
73

consider: #include "llvm/ADT/StringSet.h" & llvm::StringSet

283

hasAnyName -> hasName, there is only 1 argument.

286

hasAnyName -> hasName, there is only 1 argument.

324

change this tuple into some struct, it's hard to find out what those arguments are...

332

STYLE: no need for if (xx) { y1 } else { y2 }, just if (xx) y1 else y2; should be sufficient

341

STYLE: this else is not needed, there are returns in previous anyway
simply if will be sufficient

346

count -> contains

346

no need for tuple for 2 argments, pair would be sufficient.

350

count -> contains

clang-tools-extra/docs/ReleaseNotes.rst
155–158

maybe just "Enhanced XYZ check to support for-loops with iterators initialized by free function calls like begin, end, or size."
I got lost on this part "to refactor container based for loops"

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

what if begin,end is missing, and there is only cbegin, cend then it will fail with:
"error: invalid range expression of type 'const A'; no viable 'begin' function available|

481

shouldnt it be const& It ?

730

add some tests for C++20 with rbegin, rend

ccotter marked 10 inline comments as done.May 7 2023, 6:31 PM
ccotter added inline comments.
clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp
477

This looks to be an already existing bug in the check - mind if I fix that in a separate phab?

ccotter updated this revision to Diff 520232.May 7 2023, 6:32 PM

Feedback and rebase

ccotter marked an inline comment as done.May 7 2023, 6:57 PM
ccotter added inline comments.
clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp
481

Looks like when the object is trivial to copy (IsCheapToCopy logic in the check), auto It is used instead of const auto& It

ccotter updated this revision to Diff 520237.May 7 2023, 7:04 PM
ccotter marked an inline comment as done.
  • Fix compile for non-libc++
PiotrZSL accepted this revision.Aug 4 2023, 1:31 PM

LGTM, things that are missing:

  • some crash protection (getName)
  • test for rbegin/rend
clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
311

if we can, please use enum class

341

would be nice to verify if we got "name" here, same in other places

878

no need for else after return

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

Please add test with rbegin/rend

This revision is now accepted and ready to land.Aug 4 2023, 1:31 PM
ccotter updated this revision to Diff 547433.Aug 4 2023, 6:36 PM
ccotter marked 2 inline comments as done.
  • Feedback
ccotter updated this revision to Diff 547483.Aug 5 2023, 7:21 AM
  • Check nullptr
  • Add tests for rbegin/crbegin
ccotter marked 3 inline comments as done.Aug 5 2023, 7:25 AM
ccotter added inline comments.
clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp
477

Will do.

PiotrZSL accepted this revision.Aug 5 2023, 7:45 AM

LGTM

ccotter marked an inline comment as done.Aug 5 2023, 1:09 PM

All done on my end - @PiotrZSL if you're good would you mind landing this for me?