massberg marked 6 inline comments as done. massberg added a comment. Alex and Aaron, thanks for the comments!
================ Comment at: clang-tidy/modernize/AvoidFunctionalCheck.cpp:21 +void AvoidFunctionalCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + cxxRecordDecl(allOf(anyOf(isDerivedFrom("std::binary_function"), ---------------- aaron.ballman wrote: > 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; > }; > ``` True. This matcher covers most cases in code I have looked into. I can extend this matcher in a follow-up change. ================ Comment at: clang-tidy/modernize/AvoidFunctionalCheck.cpp:47-51 + if (!ClangDecl->getDeclName().isIdentifier() || + (ClangDecl->getName() != "unary_function" && + ClangDecl->getName() != "binary_function")) { + continue; + } ---------------- aaron.ballman wrote: > Shouldn't this be an `assert` instead? Due to multiple inheritance, there might be several bases and we have to iterate them to find the one we are interested in. I added an additional test case covering multiple inheritance. ================ Comment at: clang-tidy/modernize/AvoidFunctionalCheck.h:19 + +/// Check for several deprecated types and classes from <functional> header +/// ---------------- aaron.ballman wrote: > 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? Added full stop. I'm not sure if this check should be limited to <functional> or be extended to a full 'deprecated API' check. This change is just a start, several more types and classes which are removed from <functional> will follow, e.g: - std::ptr_fun, std::mem_fun, std::mem_fun_ref - std::bind1st, std::bind2nd - std::unary_function, std::binary_function - std::pointer_to_unary_function, std::pointer_to_binary_function, std::mem_fun_t, std::mem_fun1_t, std::const_mem_fun_t, - std::const_mem_fun1_t, std::mem_fun_ref_t, std::mem_fun1_ref_t, std::const_mem_fun_ref_t, std::const_mem_fun1_ref_t - std::binder1st, std::binder2nd As these are a bunch of functions and types, in my eyes a check just for <functional> is fine. But I'm also fine with a general 'deprecated API' check. Alex, can you comment on this? https://reviews.llvm.org/D42730 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits