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:
> 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?


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

Reply via email to