alexfh added inline comments.
================ Comment at: clang-tidy/modernize/AvoidFunctionalCheck.h:19 + +/// Check for several deprecated types and classes from <functional> header +/// ---------------- 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. https://reviews.llvm.org/D42730 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
