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

[Jens Massberg via Phabricator via cfe-commits]

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