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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
