This is an archive of the discontinued LLVM Phabricator instance.

clang-tidy: avoid std::bind
ClosedPublic

Authored by jbcoe on Feb 7 2016, 6:40 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

jbcoe retitled this revision from to clang-tidy: avoid std::bind.Feb 7 2016, 6:40 AM
jbcoe updated this object.
jbcoe added reviewers: aaron.ballman, alexfh, djasper.
jbcoe set the repository for this revision to rL LLVM.
jbcoe added a subscriber: cfe-commits.
jbcoe updated this revision to Diff 47129.Feb 7 2016, 6:40 AM
jbcoe planned changes to this revision.Feb 8 2016, 12:42 PM

Placeholder handling needs correcting.

jbcoe updated this revision to Diff 47247.Feb 8 2016, 2:31 PM
jbcoe removed rL LLVM as the repository for this revision.

Require C++14 for improved fixits.

Do not attempt to generate fixits for more complicated uses of bind.

jbcoe updated this revision to Diff 47259.Feb 8 2016, 3:00 PM

Moved check to readability module.

Aside: would it be worthwhile creating a python script to move checks from one module to another?

alexfh edited edge metadata.Feb 8 2016, 4:25 PM

Moved check to readability module.

Aside: would it be worthwhile creating a python script to move checks from one module to another?

It's reasonable to teach the rename_check.py script move across modules.

(Substantial comments later)

alexfh added inline comments.Feb 25 2016, 4:20 AM
clang-tidy/readability/AvoidStdBindCheck.cpp
37 ↗(On Diff #47259)

SmallVector<> would be better here, since the number of arguments is usually rather low.

38 ↗(On Diff #47259)

Please make the functions static.

42 ↗(On Diff #47259)

Please use a range-based for loop over C->arguments().

45 ↗(On Diff #47259)

Should be const auto *M to make it clear M is a pointer. Same for other code like this.

67 ↗(On Diff #47259)

Please use ArrayRef<> to pass a constant reference to a sequence of elements.

100 ↗(On Diff #47259)

I don't like the asymmetry here: the opening parenthesis is added in the caller and the closing parenthesis is added here.

104 ↗(On Diff #47259)
llvm::SmallPtrSet<size_t> PlaceHolderIndices;
for (const BindArgument &B : Args) {
  if (B.PlaceHolderIndex) {
    if (!PlaceHolderIndices.insert(B.PlaceHolderIndex).second)
      return true;
  }
}
return false;
126 ↗(On Diff #47259)

Many other checks use just Diag.

127 ↗(On Diff #47259)

Should the message recommend something instead?

127 ↗(On Diff #47259)

In pre-C++11 code the check will just warn without suggesting any alternative. That will lead to a lot of user confusion. We either need to restrict the warning to C++14 code or suggest a better alternative even in pre-C++14 code.

129 ↗(On Diff #47259)

nit: Trailing period.

139 ↗(On Diff #47259)

This would be shorter with std::any_of or even better with llvm::any_of that accepts a range instead of begin/end.

163 ↗(On Diff #47259)

Use llvm::any_of.

clang-tidy/readability/AvoidStdBindCheck.h
1 ↗(On Diff #47259)

Join with the next line and remove extra dashes, please.

docs/clang-tidy/checks/readability-avoid-std-bind.rst
4 ↗(On Diff #47259)

nit: Add moar ekwality.

6 ↗(On Diff #47259)

Use double backquotes to highlight inline code snippets like std::bind.

alexfh requested changes to this revision.Feb 25 2016, 4:20 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Feb 25 2016, 4:20 AM
jbcoe updated this revision to Diff 50623.Mar 14 2016, 11:48 AM
jbcoe edited edge metadata.
jbcoe marked 15 inline comments as done.

Apply requested fixes from review.

Thanks for taking time to review this, the patch is much improved for the attention.

jbcoe added inline comments.Mar 15 2016, 1:31 PM
clang-tidy/readability/AvoidStdBindCheck.cpp
42 ↗(On Diff #47259)

I'm starting at 1 not zero, hence the explicit loop.

Elements are non-contiguous so I can't use a restricted ArrayView.

I've added a comment saying why it starts at 1, not 0.

127 ↗(On Diff #47259)

The message now recommends using a lambda so I think this is addressed.

alexfh added inline comments.Mar 23 2016, 5:01 PM
clang-tidy/readability/AvoidStdBindCheck.cpp
46 ↗(On Diff #50623)

In LLVM style this should be const auto *M.
Please clang-format the file.

48 ↗(On Diff #50623)

Please use isa instead of dyn_cast, when you don't need the pointer returned by dyn_cast.

49 ↗(On Diff #50623)

I'd prefer to use the conditional operator here: B.Kind = isa<CallExpr>(TE) ? BK_CallExpr : BK_Temporary;.

116 ↗(On Diff #50623)

You should be able to use hasName("std::bind") instead.

docs/clang-tidy/checks/readability-avoid-std-bind.rst
6 ↗(On Diff #50623)

"The check finds uses of ..."

9 ↗(On Diff #50623)

Add a comma before "not"?

11 ↗(On Diff #50623)

Please add an example or two of what code is flagged and what suggested fixes are.

alexfh requested changes to this revision.Mar 23 2016, 5:01 PM
alexfh edited edge metadata.
This revision now requires changes to proceed.Mar 23 2016, 5:01 PM
aaron.ballman added inline comments.Mar 28 2016, 6:47 AM
clang-tidy/readability/AvoidStdBindCheck.cpp
116 ↗(On Diff #50623)

I think that has to be hasName("::std::bind") to support namespace mine { namespace std { void bind(); } }

126 ↗(On Diff #50623)

Might as well move this to the top of check() since there's no need to create the diagnostic or find the matched decl unless this check passes.

clang-tidy/readability/ReadabilityTidyModule.cpp
37 ↗(On Diff #50623)

I kind of wonder if this should be in modernize instead of readability? Tough call, and I'm fine with either place, just wanted to pose the question.

docs/clang-tidy/checks/readability-avoid-std-bind.rst
9 ↗(On Diff #50623)

or "and not" instead of a comma, if verbosity is your thing.

test/clang-tidy/readability-avoid-std-bind.cpp
1 ↗(On Diff #50623)

Can you clang-format this file as well?

21 ↗(On Diff #50623)

This should immediately follow the CHECK-MESSAGES in the same compound statement (here and elsewhere).

jbcoe updated this revision to Diff 55970.May 3 2016, 4:45 AM
jbcoe edited edge metadata.
jbcoe marked 16 inline comments as done.

Minor fixes from review.

clang-tidy/readability/ReadabilityTidyModule.cpp
37 ↗(On Diff #50623)

Given that lambdas and std::bind both arrived in C++11, modernize seems a misnomer.
I think readability is better fit.

jbcoe updated this revision to Diff 55974.May 3 2016, 5:11 AM
jbcoe edited edge metadata.

Remove unused function isStdBind.

alexfh added a comment.May 3 2016, 7:10 AM

Please regenerate the patch with the full context (see http://llvm.org/docs/Phabricator.html).

clang-tidy/readability/AvoidStdBindCheck.cpp
77 ↗(On Diff #55974)

clang-format, please.

116 ↗(On Diff #55974)

This should be done in registerMatchers instead.

docs/clang-tidy/checks/readability-avoid-std-bind.rst
13 ↗(On Diff #55974)

.. code:: should be followed by an empty line.

Please also verify the documentation can be built without errors. On Ubuntu this boils down to:

  1. Install sphinx:

    $ sudo apt-get install sphinx-common
  1. Enable LLVM_BUILD_DOCS and maybe some other options in cmake.
  1. ninja docs-clang-tools-html (or something similar, if you use make).
alexfh requested changes to this revision.May 3 2016, 7:11 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.May 3 2016, 7:11 AM
jbcoe requested a review of this revision.May 4 2016, 4:30 AM
jbcoe edited edge metadata.
aaron.ballman added inline comments.May 4 2016, 12:10 PM
clang-tidy/readability/ReadabilityTidyModule.cpp
17 ↗(On Diff #55974)

I believe we use "modernize" to really mean "migrate from the old way to the new way", which this definitely fits into since I think the point to this check is to replace bind with better alternatives.

jbcoe added inline comments.May 4 2016, 1:15 PM
clang-tidy/readability/ReadabilityTidyModule.cpp
17 ↗(On Diff #55974)

Would you prefer it to be in modernize? I can be easily convinced either way and am happy to move it. If I do move it I might add a script to facilitate doing so.

aaron.ballman added inline comments.May 4 2016, 1:43 PM
clang-tidy/readability/ReadabilityTidyModule.cpp
17 ↗(On Diff #55974)

My preference is for modernize, your preference is for readability, so I say: make @alexfh the tie-breaker! ;-) Alex, what are your thoughts? This seems like a heuristic we may want to state in our documentation to help others decide where to put new checks in the future as well.

jbcoe updated this revision to Diff 56273.May 5 2016, 5:52 AM
jbcoe removed a reviewer: djasper.

Move to modernize. Rename to avoid-bind as a later patch will add support for boost::bind, std::bind1st etc.

alexfh requested changes to this revision.May 11 2016, 10:34 AM
alexfh edited edge metadata.
alexfh added inline comments.
clang-tidy/modernize/AvoidBindCheck.cpp
27 ↗(On Diff #56273)

I wonder whether VS2013 supports inline member initializers?

77 ↗(On Diff #56273)

clang-format, please. Also, please join the two string literals here.

116 ↗(On Diff #56273)

This check should be done in registerMatchers to avoid unnecessary work.

161 ↗(On Diff #56273)

I don't think you need getLocForEndOfToken, since FixItHint::CreateReplacement(SourceRange) treats the range as a token range.

docs/clang-tidy/checks/modernize-avoid-bind.rst
26 ↗(On Diff #56273)

..code:: C++ directive should be followed by an empty line. Please check the documentation builds without warnings.

32 ↗(On Diff #56273)

s/We created this check because //

docs/clang-tidy/checks/readability-avoid-std-bind.rst
13 ↗(On Diff #55974)

This comment is not addressed.

test/clang-tidy/modernize-avoid-bind.cpp
25 ↗(On Diff #56273)

Remove [modernize-avoid-bind] from all CHECK-MESSAGES lines except for the first one.

This revision now requires changes to proceed.May 11 2016, 10:34 AM
jbcoe updated this revision to Diff 57022.May 12 2016, 5:41 AM
jbcoe edited edge metadata.
jbcoe marked 6 inline comments as done.

Apply fixes from review.

alexfh accepted this revision.May 12 2016, 6:01 AM
alexfh edited edge metadata.

Looks good. Thank you!

Do you need someone to submit the patch for you?

clang-tidy/modernize/AvoidBindCheck.cpp
28 ↗(On Diff #57022)

Answering my own question: according to https://msdn.microsoft.com/en-us/library/hh567368.aspx non-static member initializers are supported by VS2013. This should be fine.

This revision is now accepted and ready to land.May 12 2016, 6:01 AM
jbcoe added a comment.May 12 2016, 6:14 AM

I can submit the patch myself, thanks.

Thanks again for taking the time to give such a thorough review.

jbcoe updated this revision to Diff 57067.May 12 2016, 10:27 AM
jbcoe edited edge metadata.

update diff to allow arc to apply patch.

This revision was automatically updated to reflect the committed changes.