[PATCH] D42730: Add clang-tidy 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/AvoidFunctionalCheck.cpp:21
+void AvoidFunctionalCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      cxxRecordDecl(allOf(anyOf(isDerivedFrom("std::binary_function"),
----------------
I don't think this matcher catches other uses of these types. e.g.,
```
typedef std::binary_function<int, int, bool> awesome_sauce;
struct S : awesome_sauce {};

template <typename Ty, typename = void>
struct S {};

template <typename Ty>          
struct S<Ty, typename std::enable_if<std::is_base_of<std::binary_function<int, int, bool>, Ty>::value>::type> {
  typedef int value;
};
```


================
Comment at: clang-tidy/modernize/AvoidFunctionalCheck.cpp:22
+  Finder->addMatcher(
+      cxxRecordDecl(allOf(anyOf(isDerivedFrom("std::binary_function"),
+                                isDerivedFrom("std::unary_function")),
----------------
These should all be `::std::` instead of `std::` to cover pathological code that puts `std` inside of another namespace.


================
Comment at: clang-tidy/modernize/AvoidFunctionalCheck.cpp:37
+void AvoidFunctionalCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto* const UnaryOrBinaryDerived =
+          Result.Nodes.getNodeAs<RecordDecl>("un_or_binary_derived")) {
----------------
Formatting (here and elsewhere); you should run the patch through clang-format to handle this sort of thing.


================
Comment at: clang-tidy/modernize/AvoidFunctionalCheck.cpp:43
+      const auto *ClangDecl = BaseType->getAsCXXRecordDecl();
+      if (!ClangDecl) {
+        continue;
----------------
Should elide the braces here.


================
Comment at: clang-tidy/modernize/AvoidFunctionalCheck.cpp:47-51
+      if (!ClangDecl->getDeclName().isIdentifier() ||
+          (ClangDecl->getName() != "unary_function" &&
+           ClangDecl->getName() != "binary_function")) {
+        continue;
+      }
----------------
Shouldn't this be an `assert` instead?


================
Comment at: clang-tidy/modernize/AvoidFunctionalCheck.h:19
+
+/// Check for several deprecated types and classes from <functional> header
+///
----------------
Missing full stop at the end of the sentence.

Why should this modernize check be limited to `<functional>`? Just like we have a "deprecated headers" check, perhaps this should be a "deprecated APIs" check?


================
Comment at: docs/clang-tidy/checks/modernize-avoid-functional.rst:6
+
+In C++17 several classes, types and functions from <functional> header are no longer available.
+In particular, this check warns if one of the following deprecated objects is
----------------
"In C++17, several classes, types, and functions" (commas).


https://reviews.llvm.org/D42730