[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
+///
----------------
alexfh wrote:
> alexfh wrote:
> > 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.
> TL;DR "they do drastically different things" is the case for this check and modernize-replace-random-shuffle.
I disagree that they do drastically different things or that fix-its are a problem. Some of these APIs have replacements, others do not. At the end of the day, the basics are the same: the functionality is deprecated and you should consider a replacement. Sometimes we know that replacement up front, other times we don't. I don't think we should make users reach for a per-header file answer to that problem unless it provides them some benefit. I don't see users caring to update <functional> but not other headers.

I can see benefit to splitting the *implementations* of the checks along arbitrary lines, but how we structure the implementation is orthogonal to how we surface the functionality.


https://reviews.llvm.org/D42730