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

Fri Feb 2 02:49:45 PST 2018

alexfh added inline comments.

================
Comment at: clang-tidy/modernize/AvoidFunctionalCheck.cpp:22
+  Finder->addMatcher(
+      cxxRecordDecl(allOf(anyOf(isDerivedFrom("std::binary_function"),
+                                isDerivedFrom("std::unary_function")),
----------------
aaron.ballman wrote:
> These should all be `::std::` instead of `std::` to cover pathological code that puts `std` inside of another namespace.
Could you change the names to "::std" again, since my suggested code didn't take this comment into account?

================
Comment at: clang-tidy/modernize/AvoidFunctionalCheck.h:19
+
+/// Check for several deprecated types and classes from <functional> header
+///
----------------
aaron.ballman wrote:
> alexfh wrote:
> > aaron.ballman wrote:
> > > alexfh wrote:
> > > > Quuxplusone wrote:
> > > > > aaron.ballman wrote:
> > > > > > 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.
> > > > > This sounds like clang-tidy ought to have an umbrella option here, analogous to how -Wformat turns on -Wformat-security, -Wformat-truncation, -Wformat-overflow, etc. (Well, in GCC it doesn't, but that's the general idea.)
> > > > > So there could be a 'modernize-avoid-deprecated-in-c++11' umbrella option that turns on both 'modernize-replace-random-shuffle' and 'modernize-avoid-functional'; a 'modernize-avoid-removed-in-c++17' umbrella option that turns on those two plus some other options; and so on.
> > > > > Just a thought. If such a structure is anathema to how clang-tidy does things, then never mind. :)
> > > > > I don't see users caring to update <functional> but not other headers.
> > > > 
> > > > This is our use case: we have preferred local alternative for the deprecated APIs from <random>, so the upstream modernize-replace-random-shuffle check is out of question. However this check will be useful.
> > > > 
> > > > If the two checks are combined, we'll need to add flags to enable warnings/fixes for each subset of the APIs, which is IMO worse than having two separate checks.
> > > Why is the upstream check out of the question -- the fix-its aren't useful for your case, but it seems the check would be?
> > > 
> > > Would you be opposed to the approach @Quuxplusone suggested?
> > We have a local check that suggests the more appropriate internal APIs. The upstream check would generate duplicate warnings (and suggest conflicting fixes).
> > 
> > > Would you be opposed to the approach @Quuxplusone suggested?
> > It might be nice, but I'm not sure yet which real problem this will address. Maybe I don't see the problem due to the way I'm using clang-tidy. If the problem is unclear relationships between some checks, some of the benefits could be achieved by using more levels of hierarchy in the check names (e.g. modernize-c++11-deprecated-random-shuffle, modernize-c++11-deprecated-functional, which can be turned on together using modernize-c++11-deprecated-*).
> If the upstream check had the ability to customize the fix-it suggestions, would you still need the local check?
> 
> The problem I'd like to see addressed is that some users tend to think in terms of grouping other than header files, like "complain about everything removed in C++xy because I plan to upgrade to that at some point soon." I don't want to have those folks specify a per-header file check because the library deprecations span multiple header files with functionality that may have been deprecated or removed at different times. It seems more user friendly to have the granularity be: all deprecations, all deprecations up to this version of the standard, just deprecations for this grouping, and just this deprecation.
Looking at the code of the two versions of the random-shuffle check, I don't see how we could make the checks configurable enough to accommodate both use cases. So yes, we'd need a local check at least for this reason.

As for providing different facets of functionality (address everything deprecated in C++11/14/17/... as opposed to addressing everything in `<random>`, `<functional>`, etc.), they don't have to be in a single check each. Instead, we could use configuration facilities for this. One possibility that comes to mind is to introduce the concept of including configuration files (we have this internally, it's built on top of the `FileOptionsProvider`). Then we could provide a number of "presets" (e.g. for migration to C++17) that would enable all relevant checks and set appropriate options. Alternatively (or until we have the inclusion implemented) we could provide this kind of a configuration snippet in documentation.

WDYT?

https://reviews.llvm.org/D42730