This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add modernize check to use std::invoke in generic code
AbandonedPublic

Authored by boga95 on Sep 19 2018, 2:39 PM.

Details

Summary

In generic code (e.g.: in a type dependent expression), the std::invoke should be used, in order to support functors, function pointers, pointers to members, regular functions, and other C++ and STL features: link
The check replace the appropriate calls with calls to std::invoke. This should work only in C++17 and newer codebases.

Diff Detail

Event Timeline

boga95 created this revision.Sep 19 2018, 2:39 PM
Eugene.Zelenko added inline comments.
clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp
2

Please add space after clang-tidy.

49

Please remove empty line.

50

Please don't use auto when type could not be deduced from same statement.

54

Please don't use auto when type could not be deduced from same statement.

71

I think you should use LLVM's cast instead, but I'm not sure which one. Same for other places.

79

Please don't use auto when type could not be deduced from same statement.

97

Please remove empty line.

clang-tidy/modernize/ReplaceGenericFunctorCallCheck.h
2

Please add space after clang-tidy.

docs/ReleaseNotes.rst
60

Please sort new checks list in alphabetical order.

63

Replace -> Replaces.

docs/clang-tidy/checks/modernize-replace-generic-functor-call.rst
7

Please make first statement same as in Release Notes.

16

Please use more descriptive identifiers and separate statements with empty lines. Same for other places.

Eugene.Zelenko retitled this revision from Use std::invoke in generic code to [clang-tidy] Add modernize check to use std::invoke in generic code.Sep 19 2018, 3:35 PM
Eugene.Zelenko added inline comments.Sep 19 2018, 3:36 PM
clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp
115

Please insert empty line above.

Looks like you patch is not based on trunk. Please rebase.

docs/ReleaseNotes.rst
60

Please take a look on trunk Release Notes and use :doc: for link.

lebedev.ri added inline comments.
clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp
28–33

Bikeshedding: i do not understand the idea behind std::invoke.
Why is the new version better/preferred?

This needs a test when calling in a constexpr function. I believe std::invoke is not constepxr, so a function object call in a constexpr function should not suggest std::invoke.

aaron.ballman added inline comments.Sep 20 2018, 5:04 PM
clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp
28–33

It generically handles all the various kinds of "callable" objects (lambdas, functors, function pointers, pointer to member functions, etc).

98–100

This should use a Twine instead.

clang-tidy/modernize/ReplaceGenericFunctorCallCheck.h
20

Missing full stop at the end of the comment.

MTC added a subscriber: MTC.Sep 20 2018, 7:57 PM
lebedev.ri added inline comments.Sep 21 2018, 12:42 AM
clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp
28–33

Thank you for answering, but i still do not understand.
E.g. on that example, what benefits does it give?
Even after looking at cppreference, i'm not sure..
Just curious.

aaron.ballman added inline comments.Sep 21 2018, 10:26 AM
clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp
28–33

Genericity is the primary benefit. Here's the original paper with its motivation:

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3727.html

boga95 updated this revision to Diff 166604.Sep 22 2018, 9:36 AM
JonasToth added inline comments.
clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp
71

in this case i think const auto *BinOp = dyn_cast<BinaryOperator>(Paren->getSubExpr()); would match.

As a safety measure adding a assert(BinOp && "No Binary Operator as subexpression"); helps spotting bugs.

test/clang-tidy/modernize-replace-generic-functor-call.cpp
25

Please add tests that include the usage of macros to do the function call.
In my opinions these should be excluded from the FIXITs and only the warning should be emitted.

aaron.ballman added inline comments.Sep 24 2018, 1:58 PM
clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp
71

I agree; you'd want dyn_cast here. There's no need to add that assertion -- dyn_cast already asserts that the given value is non-null. If the value could be null and still be valid, you could use dyn_cast_or_null instead.

boga95 updated this revision to Diff 166994.Sep 25 2018, 2:40 PM
boga95 marked 12 inline comments as done.
aaron.ballman added inline comments.Sep 26 2018, 8:35 AM
clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp
71

How do you know Paren won't be null? If it cannot be null, please use cast<> instead, otherwise, you should be checking for null before dereferencing.

86

Perhaps use a Twine in the diagnose() call for this.

95–97

This is dangerous (the intermediary temps will be destroyed and you will be referencing dangling memory) -- you should lower it into the call to CreateReplacement().

99

Diagnostics should not be complete sentences, so Use -> use and drop the full stop.

test/clang-tidy/modernize-replace-generic-functor-call.cpp
24–26

Can you add a test case that demonstrates this works well in the presence of std::forward? This is a common pattern:

template <typename Callable, typename... Args>
void f(Callable&& C, Args&&... As) {
  std::forward<Callable>(C)(std::forward<Args>(As)...);
}

This could be converted into:

template <typename Callable, typename... Args>
void f(Callable&& C, Args&&... As) {
  std::invoke(C, As...);
}
boga95 updated this revision to Diff 167991.Oct 2 2018, 11:40 AM
boga95 marked 7 inline comments as done.
aaron.ballman added inline comments.Oct 3 2018, 12:02 PM
clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp
32

I don't think this will work for calls wrapped in parens or with spurious dereferences.

void f(int) {}

int main() {
  void (*fp)(int) = f;

  fp(12);
  (fp)(12);
  (*fp)(12);
}

It seems like all of those can be replaced by std::invoke() directly.

97

You might be able to get rid of the explicit Twine construction here now that Param is a Twine. I could be remembering wrong though.

gerazo added a subscriber: gerazo.Nov 26 2018, 7:21 AM
boga95 abandoned this revision.Sep 17 2019, 2:04 PM
boga95 marked 4 inline comments as done.
boga95 added inline comments.
clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp
71

It cannot be null because of the matcher.

72

I found an interesting behavior with source location:

const SourceManager *Source = Result.SourceManager;

const char *StartPos = Source->getCharacterData(Obj->getLocStart());
const char *EndPos = Source->getCharacterData(Obj->getLocEnd());

StartPos and EndPos point to the exact same location. Is this the expected behavior or it is a bug?

test/clang-tidy/modernize-replace-generic-functor-call.cpp
25

Please show me some example of that.

The review says you abandoned it -- was that accidental?

clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp
72

That is expected behavior -- the source location points to the insertion point of the token and if there's only one token in the source range, two two locations will point to the same place.

boga95 marked an inline comment as done.Sep 18 2019, 1:31 PM

Resolving the problems properly seemed quite difficult. I just finished university and started to work, therefore I don't have enough free time to finish the checker. Sorry about that.