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: > 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-*). Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42730 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
