alexfh added inline comments.
================
Comment at: clang-tidy/modernize/DeprecatedFunctionalCheck.cpp:48-54
+ } else if (const auto *const Call =
+ Result.Nodes.getNodeAs<CallExpr>("ptr_fun_call")) {
+ diag(Call->getLocStart(), Message) << "'std::ptr_fun'";
+ } else if (const auto *const Call =
+ Result.Nodes.getNodeAs<CallExpr>("mem_fun_call")) {
+ diag(Call->getLocStart(), Message) << "'std::mem_fun'";
+ }
----------------
alexfh wrote:
> aaron.ballman wrote:
> > massberg wrote:
> > > aaron.ballman wrote:
> > > > I think that this code should be generalized (same with the matchers)
> > > > so that you match on `hasAnyName()` for the function calls and use
> > > > `CallExpr::getCalleeDecl()` to get the declaration. e.g.,
> > > > ```
> > > > if (const auto *Call = Result.Nodes.getNodeAs<CallExpr>("blech")) {
> > > > if (const Decl *Callee = Call->getCalleeDecl())
> > > > diag(Call->getLocStart(), Message) << Calleee;
> > > > }
> > > > ```
> > > > This way you can add more named without having to add extra logic for
> > > > the diagnostics.
> > > I generalized the code and the matcher.
> > > When we use
> > > ```
> > > << cast<NamedDecl>(Callee);
> > > ```
> > > we get the template arguments in the name , e.g. `ptr_fun<int, int>`, so
> > > I chose to use `getQualifiedNameAsString`.
> > > If there is there a better way to get the function name without template
> > > arguments I appreciate any suggestions.
> > >
> > >
> > Nope, in that case, your code is correct. However, we generally provide the
> > template arguments in diagnostics. I see @alexfh was asking for them to be
> > removed as not being useful, but I'm not certain I agree with his
> > rationale. Yes, all instances are deprecated and thus the template
> > arguments don't discern between good and bad cases, but showing the
> > template arguments is also consistent with the other diagnostics we emit.
> > For instance, other "deprecated" diagnostics also emit the template
> > arguments. I'm not certain we should be inconsistent with the way we
> > produce diagnostics, but I'll defer to Alex if he still feels strongly
> > about leaving them off here.
> Indeed, -Wdeprecated-declarations warnings print template arguments.
> Moreover, they seem to be issued only on instantiations, see
> https://godbolt.org/g/W563gw.
>
> But I have a number of concerns with template arguments in the deprecation
> warnings:
>
> 1. The note attached to the warning lies. Consider a warning from the test
> above:
> ...
> <source>:11:1: warning: 'B<int>' is deprecated: bbb
> [-Wdeprecated-declarations]
> B<int> e;
> ^
> <source>:7:10: note: 'B<int>' has been explicitly marked deprecated here
> struct [[deprecated("bbb")]] B {};
>
> But `B<int>` hasn't been explicitly marked deprecated, only the template
> definition of `B` has been. Template arguments are important in the case of
> the explicit template specialization `A<int>` in the same example, but not in
> cases where the template definition was marked deprecated, since template
> arguments only add noise and no useful information there.
> 2. Warnings can easily become unreadable: https://godbolt.org/g/AFdznH
> 3. Certain coding patterns can result in numerous deprecation warnings
> differing only in template arguments: https://godbolt.org/g/U2JCbG.
> Clang-tidy can deduplicate warnings, if they have identical text and
> location, but adding template arguments to the message will prevent
> deduplication. I've seen a case where thousands of deprecation warnings were
> generated for a single line in a widely used header.
>
> So yes, I feel strongly about leaving off template arguments in case the
> whole template was marked deprecated. I think it would be the right thing to
> do for the -Wdeprecated-declarations diagnostic as well.
s/leaving off/leaving out/
https://reviews.llvm.org/D42730
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
