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