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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp:70
+    const auto *Paren = dyn_cast<ParenExpr>(MFunctor->getCallee());
+    const auto *BinOp = dyn_cast<BinaryOperator>(Paren->getSubExpr());
+
----------------
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.


================
Comment at: clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp:85
+
+    const std::string Param = ObjName + ", " + FuncName;
+    diagnose(MFunctor, Param, OriginalParams);
----------------
Perhaps use a `Twine` in the `diagnose()` call for this.


================
Comment at: clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp:94-96
+  const Twine &Replace = Twine("::std::invoke(") + Param +
+                         (Functor->getNumArgs() == 0 ? "" : ", ") +
+                         OriginalParams;
----------------
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()`.


================
Comment at: clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp:98
+  diag(Functor->getExprLoc(),
+       "Use ::std::invoke to invoke type dependent callable objects.")
+      << FixItHint::CreateReplacement(Functor->getSourceRange(), Replace.str());
----------------
Diagnostics should not be complete sentences, so Use -> use and drop the full stop.


================
Comment at: test/clang-tidy/modernize-replace-generic-functor-call.cpp:23-25
+template <class T>
+void func2(T func) {
+  func(1);
----------------
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...);
}
```


https://reviews.llvm.org/D52281