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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits