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'";
+ }
----------------
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.
https://reviews.llvm.org/D42730
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
