This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Prefer transparent functors to non-transparent one.
ClosedPublic

Authored by xazax.hun on Sep 24 2016, 4:00 PM.

Details

Summary

This check finds the usages of non-transparent functors and suggest to use the transparent ones.

Diff Detail

Event Timeline

xazax.hun updated this revision to Diff 72397.Sep 24 2016, 4:00 PM
xazax.hun retitled this revision from to [clang-tidy] Prefer transparent functors to non-transparent one..
xazax.hun updated this object.
xazax.hun added reviewers: alexfh, hokein.
xazax.hun set the repository for this revision to rL LLVM.
xazax.hun added a project: Restricted Project.
xazax.hun added a subscriber: cfe-commits.
aaron.ballman added inline comments.
clang-tidy/modernize/UseTransparentFunctorsCheck.cpp
27

Should we make this a configurable list that users can add to?

62

This diagnostic is too terse; anyone that is unaware of what a transparent functor is will likely be stumped by it, especially since there is no fixit.

Since this is the case where we cannot be sure that a transparent functor is the correct solution, should this be enabled via an option (default on)?

94

This diagnostic could stand to be less terse as well, IMO. The fixit helps though.

docs/clang-tidy/checks/modernize-use-transparent-functors.rst
7

Using transparent functors the -> When using transparent functors, the

xazax.hun updated this revision to Diff 75530.Oct 22 2016, 9:26 AM
xazax.hun updated this object.
xazax.hun removed rL LLVM as the repository for this revision.
xazax.hun marked 3 inline comments as done.
  • Fixed the performance problems.
  • Altered the diagnostic text.
  • Documentation improvements.
  • Added an option to silence some warnings.
  • Updated to latest trunk.
clang-tidy/modernize/UseTransparentFunctorsCheck.cpp
27

I am not sure how frequent is that somebody would like to add some types to this list, but it can be added in a follow up patch.

62

I also extended the error message to refer to the alternative name (diamond operators) as well.

I did add an option but I am not happy with the name of the option. Do you have a suggestion?

xazax.hun added a comment.EditedOct 22 2016, 9:42 AM

I attached the results of this check for LLVM.

Prazek added a subscriber: Prazek.Oct 23 2016, 2:58 AM
Prazek added inline comments.
clang-tidy/modernize/UseTransparentFunctorsCheck.cpp
71

The message would be much better if you would put the name of this functor, like
"prefer transparent functor (%0)" where %0 would be evaluated to 'std::greater<>" etc.

90–110

This can be moved to one or 2 functions, returning FunctorTypeLoc or llvm::Optional<TemplateSpecializationTypeLoc>

docs/clang-tidy/checks/modernize-use-transparent-functors.rst
33–34

I think
... will not warn on these cases as shown above, where automatic FIXIT...
would have been better.

34

Add a note
This check requires C++14 or higher to run.

Prazek added inline comments.Oct 23 2016, 3:00 AM
docs/clang-tidy/checks/modernize-use-transparent-functors.rst
13

Say somewhere that you also handle cases like
std::less<Type>(arg1, arg2)
because from this documentation I didn't know about it.

aaron.ballman added inline comments.Oct 26 2016, 11:56 AM
clang-tidy/modernize/UseTransparentFunctorsCheck.cpp
27

I'm fine with a follow-on patch.

62

SafeMode is a bit dramatic-sounding, but I can't come up with something better, so it's probably fine.

xazax.hun updated this revision to Diff 76860.Nov 3 2016, 8:01 AM
xazax.hun marked 5 inline comments as done.
  • Addressed review comments.
aaron.ballman added inline comments.Nov 10 2016, 8:37 AM
clang-tidy/modernize/UseTransparentFunctorsCheck.cpp
72

You should quote the %0 to clarify that you're referring to syntax.

90

Is the .str() required? (Same question applies below.)

xazax.hun updated this revision to Diff 77964.Nov 15 2016, 2:44 AM
  • Updated to latest TOT
  • Fixed review comments.
xazax.hun marked 2 inline comments as done.Nov 15 2016, 2:45 AM
xazax.hun added inline comments.
clang-tidy/modernize/UseTransparentFunctorsCheck.cpp
90

It does not compile without it. It looks like the << is not overloaded for Twine.

aaron.ballman accepted this revision.Nov 15 2016, 11:16 AM
aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added inline comments.
clang-tidy/modernize/UseTransparentFunctorsCheck.cpp
90

I learn something new every day. ;-)

docs/clang-tidy/checks/modernize-use-transparent-functors.rst
8

It not possible -> It is not possible

35–37

The way I read this, it seems to be claiming that if you set SafeMode to nonzero, the check never warns. How about:

If the option is set to non-zero, the check will not diagnose cases where using a transparent functor cannot be guaranteed to produce identical results as the original code. The default value for this option is `0`.
This revision is now accepted and ready to land.Nov 15 2016, 11:16 AM
xazax.hun closed this revision.Nov 16 2016, 6:53 AM
xazax.hun marked 2 inline comments as done.

Thanks for the reviews, committed in https://reviews.llvm.org/rL287107 .