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.
Details
- Reviewers
alexfh hokein aaron.ballman
Diff Detail
Event Timeline
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. |
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. |
clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp | ||
---|---|---|
28–33 | Bikeshedding: i do not understand the idea behind std::invoke. |
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.
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. |
clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp | ||
---|---|---|
28–33 | Thank you for answering, but i still do not understand. |
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 |
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 | ||
24 | Please add tests that include the usage of macros to do the function call. |
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. |
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...); } |
clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp | ||
---|---|---|
33 | 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. | |
98 | 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. |
clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp | ||
---|---|---|
71 | It cannot be null because of the matcher. | |
73 | 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 | ||
24 | Please show me some example of that. |
The review says you abandoned it -- was that accidental?
clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp | ||
---|---|---|
73 | 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. |
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.
Please add space after clang-tidy.