aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.
A few comments were not applied, and I'd like a more descriptive name for the
check (especially if we plan to generalize this).
================
Comment at: clang-tidy/modernize/AvoidFunctionalCheck.h:19
+
+/// Check for several deprecated types and classes from <functional> header
+///
----------------
alexfh wrote:
> aaron.ballman wrote:
> > alexfh wrote:
> > > 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?
> > > 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.
> >
> > Good to know -- thank you for sharing your usage experience!
> >
> > > WDYT?
> >
> > I might be misunderstanding how the user will interact with configuration
> > files to get this behavior, but... why should this require configuration
> > files rather than use what users are already used to in warning groups? My
> > gut feeling is that we should be surfacing features in ways the user is
> > already familiar with, and this seems somewhat novel for warnings compared
> > to the grouping approach. This approach means the user experience isn't
> > always going to be uniform like with warning groups -- different
> > configurations provide different behaviors. Also, there's the increased
> > cognitive expense of users finding out about these configuration files and
> > editing them manually to get the behavior they need.
> >
> > We already have the notion of check aliases that can be reconfigured
> > as-needed, so what I was envisioning were pre-built "checks" that are
> > simply an alias to other checks, appropriately configured. To the user, it
> > looks and acts just like any other check, including allowing the user to
> > specify "warn me about all the C++17 deprecations except this one".
> I'm happy to continue the discussion, but I'd like to unblock this patch.
> Whichever mechanism we choose or come up with, it may take some time. But I'd
> like to be able to use this check sooner.
>
> I hope, this is fine by you.
> I'm happy to continue the discussion, but I'd like to unblock this patch.
> Whichever mechanism we choose or come up with, it may take some time. But I'd
> like to be able to use this check sooner.
>
> I hope, this is fine by you.
Yup, fine by me to unblock this patch.
================
Comment at: clang-tidy/modernize/ModernizeTidyModule.cpp:50
+ CheckFactories.registerCheck<AvoidFunctionalCheck>(
+ "modernize-avoid-functional");
CheckFactories.registerCheck<DeprecatedHeadersCheck>(
----------------
I'm not keen on this name -- it suggests the user should avoid functional,
which is certainly not the advice we want to give them. Also, it makes no
mention of deprecations, which is what the check is all about.
How about: `modernize-functional-deprecations` and we can use
`modernize-deprecations` as the eventual catch-all umbrella, `modernize-<header
name>-deprecations` for other headers, and `modernize-<header
name>-deprecations-<api>` if we want to add API-level granularity?
================
Comment at: docs/ReleaseNotes.rst:95
+
+ Warns if types, classes and functions from '<functional>' header which are
+ deprecated in C++11 and removed in C++17 are used.
----------------
type, classes and functions -> types, classes, and functions
================
Comment at: docs/clang-tidy/checks/modernize-avoid-functional.rst:10
+
+- std::unary_function
+- std::binary_function
----------------
Eugene.Zelenko wrote:
> Please enclose names in ``.
This comment was not properly applied -- it was calling for backticks, not a
single quote.
================
Comment at: docs/clang-tidy/checks/modernize-avoid-functional.rst:6
+
+In C++17 several classes, types and functions from <functional> header are no
longer available.
+In particular, this check warns if one of the following deprecated objects is
----------------
aaron.ballman wrote:
> "In C++17, several classes, types, and functions" (commas).
This comment was not applied but is marked as done. Missing the Oxford comma
still.
https://reviews.llvm.org/D42730
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
