Page MenuHomePhabricator

[clang-tidy] Repair various issues with modernize-avoid-bind
ClosedPublic

Authored by jaafar on Jun 12 2020, 2:48 PM.

Details

Summary

In the process of running this check on a large codebase I found a number of limitations, and thought I would pass on my fixes for possible integration upstream:

  1. Templated function call operators are not supported
  2. Function object constructors are always used directly in the lambda body, even if their arguments are not captured
  3. Placeholders with namespace qualifiers (std::placeholders::_1) are not detected
  4. Lambda arguments should be forwarded to the stored function
  5. Data members from other classes still get captured with this
  6. Expressions (as opposed to variables) inside std::ref are not captured properly
  7. Function object templates sometimes have their template arguments replaced with concrete types

This patch resolves all those issues and adds suitable unit tests.

If desired, I can separate these commits out into 7 separate diffs, but it seemed like it might be easier to evaluate them all at once.

Diff Detail

Event Timeline

jaafar created this revision.Jun 12 2020, 2:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 12 2020, 2:48 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
jaafar marked 3 inline comments as done.Jun 12 2020, 3:05 PM
jaafar added inline comments.
clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
38–39

"Captured by reference" and "uses init expression" are not mutually exclusive - that gave rise to item 6 above.

156

These forward declarations seemed to make the diffs a lot easier to read. It's also possible to move the two functions into this position, of course.

244

Is there a better way to express this? "a single child of type ThisExpr" was what I was going for...

Eugene.Zelenko retitled this revision from Repair various issues with modernize-avoid-bind to [clang-tidy] Repair various issues with modernize-avoid-bind.Jun 12 2020, 7:58 PM
Eugene.Zelenko added a reviewer: njames93.

"Ping". I hope this can be considered for 10.0.1. If nothing else I think reviewing the test cases has a lot of value - there are some real issues with the current checks and fixits.

"Ping". I hope this can be considered for 10.0.1. If nothing else I think reviewing the test cases has a lot of value - there are some real issues with the current checks and fixits.

Thank you for all of these fixes! I like the direction it's going in, but am a bit wary of introducing this for 10.0.1 because it changes a lot of behaviors (not to say it shouldn't be 10.0.1, just to express a bit of trepidation).

clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
156

I think it's fine to use forward declares, but I'd prefer them just after the anonymous namespace (or before it) so the declarations are towards the top of the file instead of stuck in the middle.

167

Comments should be grammatical, so include capitalization and punctuation. (Here and elsewhere in the patch.)

244

if (isa<CXXThisExpr>(ME->getBase())) -- MemberExpr::children() only considers the base anyway, so there's only ever one child node to begin with.

282–285

const auto * since the type is spelled out in the initialization.

414–418

This doesn't need to be done in two steps. Better:

const auto *FTD = dyn_cast<FunctionTemplateDecl>(D);
if (!FTD)
  continue;

const FunctionDecl *FD = FTD->getTemplatedDecl();
jaafar marked an inline comment as done.Jun 22 2020, 5:46 PM
jaafar added inline comments.
clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
244

Could there be zero? I'm just worried about dereferencing ME->children().begin() before verifying there is at least one...

jaafar updated this revision to Diff 272580.Jun 22 2020, 5:50 PM

Applied feedback from Aaron Ballman. Thanks!

jaafar marked 4 inline comments as done.Jun 22 2020, 5:52 PM
njames93 added inline comments.Jun 23 2020, 3:52 AM
clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
244

The child nodes in MemberExpr just point to one node, its base, which should never be null. Is it not simpler to just use this?

if (isa<CXXThisExpr>(ME->getBase())) {
jaafar updated this revision to Diff 272674.Jun 23 2020, 4:25 AM

Applied feedback from Nathan James

jaafar marked 3 inline comments as done.Jun 23 2020, 4:26 AM
jaafar added inline comments.
clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
244

Ah, much better, thanks!

aaron.ballman accepted this revision.Jun 23 2020, 8:26 AM

Aside from a minor nit, this LGTM

clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
414–415

You can drop this entirely -- the dyn_cast<> below does the work for you.

This revision is now accepted and ready to land.Jun 23 2020, 8:26 AM
jaafar updated this revision to Diff 272813.Jun 23 2020, 1:47 PM
jaafar marked an inline comment as done.

One more simplification from Aaron. Thanks!

jaafar marked an inline comment as done.Jun 23 2020, 1:48 PM

One more simplification from Aaron. Thanks!

No problem! Do you need someone to commit on your behalf, btw? If so, let me know what name and email address you would like me to attribute.

I do need someone to commit this, yes, thank you.

I'm Jeff Trull edaskel@att.net

aaron.ballman closed this revision.Jun 25 2020, 4:33 AM

Thank you for the patch! I've commit on your behalf in 95a3550dc89a0d424d90e2c0ad30d9ecfa9422cf