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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.


================
Comment at: clang-tidy/modernize/AvoidFunctionalCheck.h:19
+
+/// Check for several deprecated types and classes from <functional> header
+///
----------------
aaron.ballman wrote:
> 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.
> I'm not sure if this check should be limited to <functional> or be extended to a full 'deprecated API' check.

IIUC, it should be possible to implement fixits at least for some use cases here. My impression was that Jens was at least considering to work on fixits. The other check mentioned here - `modernize-replace-random-shuffle` already implements fixits. Fixits are specific to the API and some codebases may have better replacement APIs than what the standard suggests, so different users may want to apply different set of the fixes. Given all that, I wouldn't just merge all of the checks dealing with deprecated APIs. Splitting them at least by header seems like a good start, maybe even finer granularity may be needed in some cases.


https://reviews.llvm.org/D42730