[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.h:19
+
+/// Check for several deprecated types and classes from <functional> header
+///
----------------
massberg wrote:
> massberg wrote:
> > 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?
> There are already other checks for functions which are removed in C++17 like modernize-replace-random-shuffle.
> So I think having an separate check for functions and types removed from <functional> would be OK.
You've hit the nail on the head for what I'm trying to avoid -- we shouldn't have multiple checks unless they do drastically different things. Having a deprecated check like this really only makes sense for APIs that are deprecated but aren't uniformly marked as `[[deprecated]]` by the library. As such, I think we really only need one check for this rather than splitting it out over multiple checks -- the existing check functionality could be rolled into this one and its check become an alias.


https://reviews.llvm.org/D42730