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:
> alexfh wrote:
> > 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/
> > The note attached to the warning lies.
>
> No it doesn't? The attribute is inherited from the primary template unless
> your explicit specialization *removes* the attribute.
> https://godbolt.org/g/ZuXZds
>
> > Warnings can easily become unreadable
>
> This is true of all template diagnostics and isn't specific to clang-tidy's
> treatment of them.
>
> > I've seen a case where thousands of deprecation warnings were generated for
> > a single line in a widely used header.
>
> This sounds more worrying, but at the same time, your link behaving by design
> and doing what I would want it to do. The presence of the deprecated primary
> template isn't what gets diagnosed, it's the *uses* of the deprecated entity.
> This is called out explicitly in [dcl.attr.deprecated]p4.
>
> > 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.
>
> I would be strongly opposed to that change to -Wdeprecated-declarations.
>
> We may be at an impasse here, but my viewpoint is unchanged -- I think
> removing the template arguments is inconsistent with other diagnostics. I'll
> defer to you on this, but I think it's a mistake and definitely do not want
> to see it used as precedent.
Let's try to look at this from a different angle: are there benefits (apart
from consistency) of including template arguments to deprecation warnings where
the primary template is deprecated rather than a specialization? Could you
provide an example of a case where template arguments are making the warning
easier to understand or to act upon?
> The presence of the deprecated primary template isn't what gets diagnosed,
> it's the *uses* of the deprecated entity. This is called out explicitly in
> [dcl.attr.deprecated]p4.
Sure, I'm not proposing to change _where_ the warnings are produced, I just
want the warnings to be free of unnecessary information that unnecessarily
makes the warning messages different. In the example I provided
(https://godbolt.org/g/U2JCbG) the program only refers to the deprecated entity
(class Q) once after it's declared (`Q<T>` in `class S : Q<T> {};`). IMO
knowing the specific value of `T` doesn't give the user any useful information
in this case. This only makes the message less readable and gets in the way of
any efforts to deduplicate the warnings. Am I missing something?
https://reviews.llvm.org/D42730
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
