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

[boga95]

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.

[Eugene.Zelenko]

clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp

1

Please add space after clang-tidy.

48

Please remove empty line.

49

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

53

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

70

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

78

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

96

Please remove empty line.

clang-tidy/modernize/ReplaceGenericFunctorCallCheck.h

1

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

6

Please make first statement same as in Release Notes.

15

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

[Eugene.Zelenko]

clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp

114

Please insert empty line above.

[Eugene.Zelenko]

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]

clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp

27–32

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

[pfultz2]

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]

clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp

27–32

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

97–99

This should use a Twine instead.

clang-tidy/modernize/ReplaceGenericFunctorCallCheck.h

19

Missing full stop at the end of the comment.

[lebedev.ri]

clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp

27–32

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]

clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp

27–32

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

[JonasToth]

clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp

70

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]

clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp

70

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.

[aaron.ballman]

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...);
}

[aaron.ballman]

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.

[boga95]

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

25

Please show me some example of that.

[aaron.ballman]

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.

[boga95]

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.

clang-tidy/modernize/CMakeLists.txt

   RawStringLiteralCheck.cpp
   RedundantVoidArgCheck.cpp
   ReplaceAutoPtrCheck.cpp
+  ReplaceGenericFunctorCallCheck.cpp
   ReplaceRandomShuffleCheck.cpp
   ReturnBracedInitListCheck.cpp
   ShrinkToFitCheck.cpp

clang-tidy/modernize/ModernizeTidyModule.cpp

 #include "RawStringLiteralCheck.h"
 #include "RedundantVoidArgCheck.h"
 #include "ReplaceAutoPtrCheck.h"
+#include "ReplaceGenericFunctorCallCheck.h"
 #include "ReplaceRandomShuffleCheck.h"
 #include "ReturnBracedInitListCheck.h"
 #include "ShrinkToFitCheck.h"

         "modernize-redundant-void-arg");
     CheckFactories.registerCheck<ReplaceAutoPtrCheck>(
         "modernize-replace-auto-ptr");
+    CheckFactories.registerCheck<ReplaceGenericFunctorCallCheck>(
+        "modernize-replace-generic-functor-call");
     CheckFactories.registerCheck<ReplaceRandomShuffleCheck>(
         "modernize-replace-random-shuffle");
     CheckFactories.registerCheck<ReturnBracedInitListCheck>(

clang-tidy/modernize/ReplaceGenericFunctorCallCheck.h

+//===--- ReplaceGenericFunctorCallCheck.h - clang-tidy-----------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_REPLACEGENERICFUNCTORCALLCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_REPLACEGENERICFUNCTORCALLCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+/// Use std::invoke to call callable objects in generic code
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-replace-generic-functor-call.html
+class ReplaceGenericFunctorCallCheck : public ClangTidyCheck {
+public:
+  ReplaceGenericFunctorCallCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  void diagnose(const CallExpr *Functor, const std::string &Param,
+                const std::string &OriginalParam);
+  std::string strFromLoc(const SourceManager *Source,
+                         const SourceLocation &Start,
+                         const SourceLocation &End);
+};
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_REPLACEGENERICFUNCTORCALLCHECK_H

clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp

+//===--- ReplaceGenericFunctorCallCheck.cpp - clang-tidy-------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "ReplaceGenericFunctorCallCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include <iostream>
+#include <sstream>
+#include <string>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+void ReplaceGenericFunctorCallCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus17)
+    return;
+
+  // template<class T>
+  // void f(T func) {
+  //   func();
+  //   ^~~~~~~
+  //   ::std::invoke(func, 1)
+  Finder->addMatcher(callExpr(has(declRefExpr())).bind("functor"), this);
+
+  // template<class T>
+  // void f(T mfp) {
+  //   Foo foo;
+  //   (foo.*mfp)();
+  //   ^~~~~~~~~~~~~
+  //   ::std::invoke(foo, func, 1)
+  Finder->addMatcher(
+      callExpr(has(parenExpr(has(binaryOperator(hasOperatorName(".*"))))))
+          .bind("mfunctor"),
+      this);
+}
+
+void ReplaceGenericFunctorCallCheck::check(
+    const MatchFinder::MatchResult &Result) {
+
+  const auto *Source = Result.SourceManager;
+
+  const auto *Functor = Result.Nodes.getNodeAs<CallExpr>("functor");
+  if (Functor && Functor->isTypeDependent()) {
+    const auto EndLoc = Functor->getNumArgs() == 0
+                            ? Functor->getRParenLoc()
+                            : Functor->getArg(0)->getLocStart();
+    const std::string Param =
+        strFromLoc(Source, Functor->getLocStart(), EndLoc.getLocWithOffset(-1));
+    const std::string OriginalParams =
+        Functor->getNumArgs() == 0
+            ? ""
+            : strFromLoc(Source, Functor->getArg(0)->getLocStart(),
+                         Functor->getLocEnd());
+
+    diagnose(Functor, Param, OriginalParams + ")");
+    return;
+  }
+
+  const auto *MFunctor = Result.Nodes.getNodeAs<CallExpr>("mfunctor");
+  if (MFunctor && MFunctor->isTypeDependent()) {
+    const auto *Paren = static_cast<const ParenExpr *>(MFunctor->getCallee());
+    const auto *BinOp =
+        static_cast<const BinaryOperator *>(Paren->getSubExpr());
+
+    const auto *Obj = BinOp->getLHS();
+    const std::string ObjName =
+        strFromLoc(Source, Obj->getLocStart(), BinOp->getOperatorLoc());
+
+    const auto *MFunc = BinOp->getRHS();
+    const char *MFuncEnd = Source->getCharacterData(Paren->getRParen());
+    const std::string FuncName =
+        strFromLoc(Source, MFunc->getLocStart(), Paren->getRParen());
+
+    std::string OriginalParams(MFuncEnd + 2,
+                               Source->getCharacterData(MFunctor->getLocEnd()) -
+                                   (MFuncEnd + 1));
+
+    const std::string Param = ObjName + ", " + FuncName;
+    diagnose(MFunctor, Param, OriginalParams);
+    return;
+  }
+}
+
+void ReplaceGenericFunctorCallCheck::diagnose(
+    const CallExpr *Functor, const std::string &Param,
+    const std::string &OriginalParams) {
+
+  std::ostringstream Replace;
+  Replace << "::std::invoke(" << Param
+          << (Functor->getNumArgs() == 0 ? "" : ", ") << OriginalParams;
+  diag(Functor->getExprLoc(),
+       "Use ::std::invoke to invoke type dependent callable objects.")
+      << FixItHint::CreateReplacement(Functor->getSourceRange(), Replace.str());
+}
+
+std::string
+ReplaceGenericFunctorCallCheck::strFromLoc(const SourceManager *Source,
+                                           const SourceLocation &Start,
+                                           const SourceLocation &End) {
+  const char *StartPos = Source->getCharacterData(Start);
+  const char *EndPos = Source->getCharacterData(End);
+  int Len = EndPos - StartPos >= 0 ? EndPos - StartPos : 0;
+  return std::string(StartPos, Len);
+}
+} // namespace modernize
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

 Improvements to clang-tidy
 --------------------------
 
+- New `modernize-replace-generic-functor-call
+  <http://clang.llvm.org/extra/clang-tidy/checks/modernize-replace-generic-functor-call.html>`_ check
+
+  Replace type dependent functor, function pointer and pointer to member call
+  to the proper ``std::invoke()`` in C++17 or newer codebases.
+
 - The 'misc-incorrect-roundings' check was renamed to `bugprone-incorrect-roundings
   <http://clang.llvm.org/extra/clang-tidy/checks/bugprone-incorrect-roundings.html>`_
 

docs/clang-tidy/checks/list.rst

    modernize-raw-string-literal
    modernize-redundant-void-arg
    modernize-replace-auto-ptr
+   modernize-replace-generic-functor-call
    modernize-replace-random-shuffle
    modernize-return-braced-init-list
    modernize-shrink-to-fit

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

+.. title:: clang-tidy - modernize-replace-generic-functor-call
+
+modernize-replace-generic-functor-call
+======================================
+
+This check finds and replace the functor, function pointer and pointer to member
+calls to the proper ``std::invoke()`` call. It works only in C++17 or newer.
+
+Examples:
+
+.. code-block:: c++
+
+  template<class T1, class T2>
+  void f(T1 func, T2 mfp) {
+    func(2); // Replace to ::std::invoke(func, 2)
+    Foo foo;
+    (foo.*mfp)(1, 2); // Replace to ::std::invoke(foo, mfp, 1, 2)
+    (Foo().*mfp)(3); // Replace to ::std::invoke(Foo(), mfp, 3)
+  }
+
+  // Neither of them is type dependent, no diagnose
+  void g(std::function func, int (*fp)(int), void (Foo::*mfp)(int)) {
+    func3();
+    std::max(1,2);
+    Foo::print_();
+    [](){}();
+    func();
+    fp(3);
+    Foo foo;
+    (foo.*(mfp))(1);
+  }

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

+// RUN: %check_clang_tidy %s modernize-replace-generic-functor-call %t -- -- -std=c++17
+
+namespace std {
+
+template <class T>
+T max(T a, T b) {
+  return a < b ? b : a;
+}
+
+struct function {
+  void operator()();
+};
+
+} // namespace std
+
+struct Foo {
+  static void print_() {}
+  void print() const {}
+  int num_;
+};
+
+// Something that triggers the check
+template <class T>
+void func2(T func) {
+  func(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Use ::std::invoke to invoke type dependent callable objects. [modernize-replace-generic-functor-call]
+  // CHECK-FIXES: ::std::invoke(func, 1);
+  func();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Use ::std::invoke to invoke type dependent callable objects. [modernize-replace-generic-functor-call]
+  // CHECK-FIXES: ::std::invoke(func);
+
+  Foo foo;
+  (foo.*func)(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Use ::std::invoke to invoke type dependent callable objects. [modernize-replace-generic-functor-call]
+  // CHECK-FIXES: ::std::invoke(foo, func, 1);
+  (foo.*func)();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Use ::std::invoke to invoke type dependent callable objects. [modernize-replace-generic-functor-call]
+  // CHECK-FIXES: ::std::invoke(foo, func);
+  (Foo().*func)(1, 2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Use ::std::invoke to invoke type dependent callable objects. [modernize-replace-generic-functor-call]
+  // CHECK-FIXES: ::std::invoke(Foo(), func, 1, 2);
+}
+
+// Something that doesn't triggers the check
+void func3() {}
+
+void g(std::function func, int (*fp)(int), void (Foo::*mfp)(int)) {
+  func3();        // Regular function call
+  std::max(1, 2); // Template function call
+  Foo::print_();  // Static function call
+  []() {}();      // Lambda call
+  func();         // Call through std::function
+  fp(3);          // Call through function pointer
+  Foo foo;
+  (foo.*(mfp))(1); // Call through member function pointer
+}