This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Rewrite modernize-avoid-bind check
ClosedPublic

Authored by zturner on Nov 17 2019, 6:34 PM.

Details

Summary

This represents largely a full re-write of modernize-avoid-bind, adding significant new functionality in the process. In particular:

  1. Both boost::bind and std::bind are now supported
  2. Function objects are supported in addition to functions
  3. Member functions are supported
  4. Nested calls are supported using capture-init syntax
  5. std::ref() and boost::ref() are now recognized, and will capture by reference.
  6. Rather than capturing with a global =, we now build up an individual capture list that is both necessary and sufficient for the call.
  7. Fixits are supported in a much larger variety of scenarios than before.

All previous tests pass under the re-write, but a large number of new tests have been added as well.

I don't know who the best person to review this is, so I've put a couple of people on here. Feel free to re-direct if there's someone better.

Diff Detail

Event Timeline

zturner created this revision.Nov 17 2019, 6:34 PM

Please describe changes in documentation and Release Notes.

Eugene.Zelenko edited reviewers, added: alexfh, hokein; removed: dblaikie, jbcoe, NoQ.Nov 17 2019, 9:59 PM
Eugene.Zelenko set the repository for this revision to rG LLVM Github Monorepo.
Eugene.Zelenko added a project: Restricted Project.
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2019, 9:59 PM

This is a great re-write, thank you for working on it!

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

You can remove some newlines here.

112

What about CXXBindTemporaryExpr?

Would Expr::IgnoreImplicit() do too much stripping because it removes FullExpr as well?

120

Spurious newline

126

Similar question here about bound temporaries as above.

139–143

llvm::utostr() instead of using a streaming interface?

140

I think you can drop the llvm:: here and elsewhere in the file.

149

This should probably use cast<> if it's going to assume the returned value is never null.

192–195

return llvm::any_of(Statement->children(), anyDescendantIsLocal);?

382–385

It seems like return Candidates; will suffice. :-)

zturner marked 2 inline comments as done.Nov 18 2019, 3:09 PM
zturner added inline comments.
clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
112

I don't actually know. This patch is literally my first exposure to clang's frontend and AST manipulations, so I was kind of just trying different things until something worked here. I didn't even know about IgnoreImplicit() or FullExpr. I'll play around with them and report back.

149

Good point. Since we're talking about this code anyway, it felt super hacky to instantiate an AST matcher just to check for the qualified name of a Decl. Is there a better way to do this?

zturner updated this revision to Diff 230161.Nov 19 2019, 3:37 PM

Updated with suggestions from @aaron.ballman

zturner updated this revision to Diff 230162.Nov 19 2019, 3:43 PM

Forgot to remove spurious llvm:: namespace qualifications.

aaron.ballman added inline comments.Nov 20 2019, 7:24 AM
clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
149

Since you only care about named call declarations, I think you could probably get away with:

if (const auto *ND = dyn_cast<NamedDecl>(CE->getCalleeDecl())) {
  const std::string &Str = ND->getQualifiedNameAsString();
  if (Str == "::boost::ref" || Str == "::std::ref") {
    ...
  }
}
167

isa<CXXThisExpr>(Statement)

430–431

I think this can be replaced with: return dyn_cast<FunctionDecl>(DRE->getDecl());

zturner updated this revision to Diff 230495.Nov 21 2019, 11:01 AM
  • Updated documentation for this check
  • Incorporated additional suggestions from @aaron.ballman
  • Fixed an invalid transformation that was generated when binding a member function and the second argument of bind (the object pointer) was a placeholder. Test is added for this case as well.
  • Fixed an invalid transformation that was generated when a placeholder index was entirely skipped, as in the call std::bind(add, 0, _2); In this case we need to generate an unused placeholder in the first position of the resulting lambda's parameter list.
  • Added a clang-tidy option PermissiveParameterList which appends auto&&... to the end of every lambda's placeholder list. This is necessary in some situations to prevent clang-tidy from applying a fixit that causes the code to no longer compile.

Changes should be also reflected in Release Notes.

clang-tools-extra/docs/clang-tidy/checks/modernize-avoid-bind.rst
7 ↗(On Diff #230495)

Please fix double space. Same in other places.

12 ↗(On Diff #230495)

Please use double back-ticks for bind.

13 ↗(On Diff #230495)

fixit -> fix-it.

49 ↗(On Diff #230495)

Please use double back-ticks for auto&&....

50 ↗(On Diff #230495)

fixit -> fix-it.

51 ↗(On Diff #230495)

Please use double back-ticks for bind.

64 ↗(On Diff #230495)

Please use double back-ticks for ignore_args.

75 ↗(On Diff #230495)

Please use double back-ticks for operator().

zturner updated this revision to Diff 230661.Nov 22 2019, 8:51 AM

Addressed suggestions from @Eugene.Zelenko

Eugene.Zelenko added inline comments.Nov 22 2019, 9:04 AM
clang-tools-extra/docs/ReleaseNotes.rst
194 ↗(On Diff #230661)

Please use single back-tics for options.

clang-tools-extra/docs/clang-tidy/checks/modernize-avoid-bind.rst
50 ↗(On Diff #230661)

Double space here and in other places are still not fixed. Same for fixit.

Please mark addressed comments as done.

aaron.ballman accepted this revision.Nov 23 2019, 12:58 PM
aaron.ballman marked an inline comment as done.

LGTM once you handle the last remaining nits from @Eugene.Zelenko.

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

Unless you find some reason why they're critical in the initial version of your patch, I'm fine handling this as a follow-up if needed.

This revision is now accepted and ready to land.Nov 23 2019, 12:58 PM
This revision was automatically updated to reflect the committed changes.