[PATCH] D42730: [clang-tidy]] Add check for use of types/classes/functions from <functional> header which are deprecated and removed in C++17

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/modernize/DeprecatedFunctionalCheck.cpp:26
+                                            .bind("base")),
+                          anyOf(isClass(), ast_matchers::isStruct()),
+                          ast_matchers::isDefinition()))
----------------
You can drop the `ast_matchers::` here because of the using declaration above.

Actually, do we need the `isClass()` and `isStruct()` check at all? What else would match `isDerivedFrom()`?


================
Comment at: clang-tidy/modernize/DeprecatedFunctionalCheck.cpp:30
+      this);
+  Finder->addMatcher(callExpr(callee(functionDecl(hasName("std::ptr_fun"))))
+                         .bind("ptr_fun_call"),
----------------
::std::ptr_fun


================
Comment at: clang-tidy/modernize/DeprecatedFunctionalCheck.cpp:33
+                     this);
+  Finder->addMatcher(callExpr(callee(functionDecl(hasName("std::mem_fun"))))
+                         .bind("mem_fun_call"),
----------------
::std::mem_fun

Why not `::std::mem_fun_ref`? Or the _t variants of these APIs?


================
Comment at: clang-tidy/modernize/DeprecatedFunctionalCheck.cpp:48-54
+  } else if (const auto *const Call =
+                 Result.Nodes.getNodeAs<CallExpr>("ptr_fun_call")) {
+    diag(Call->getLocStart(), Message) << "'std::ptr_fun'";
+  } else if (const auto *const Call =
+                 Result.Nodes.getNodeAs<CallExpr>("mem_fun_call")) {
+    diag(Call->getLocStart(), Message) << "'std::mem_fun'";
+  }
----------------
I think that this code should be generalized (same with the matchers) so that you match on `hasAnyName()` for the function calls and use `CallExpr::getCalleeDecl()` to get the declaration. e.g.,
```
if (const auto *Call = Result.Nodes.getNodeAs<CallExpr>("blech")) {
  if (const Decl *Callee = Call->getCalleeDecl())
    diag(Call->getLocStart(), Message) << Calleee;
}
```
This way you can add more named without having to add extra logic for the diagnostics.


================
Comment at: docs/ReleaseNotes.rst:95
+
+  Warns if types, classes, and functions from '<functional>' header which are
+  deprecated in C++11 and removed in C++17 are used.
----------------
Please use backticks for `<functional>`. Also, should say "and functions from the <functional> header".


================
Comment at: docs/clang-tidy/checks/modernize-deprecated-functional.rst:6
+
+Warns if types, classes, and functions from '<functional>' header which are deprecated in C++11 and removed in C++17 are used.
+In particular, this check warns if one of the following deprecated objects is
----------------
Same comments here as above.


https://reviews.llvm.org/D42730