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: > aaron.ballman wrote: > > alexfh wrote: > > > 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? > > > 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? > > > > My concern is that we elide the template args in *all* cases, not just > > primary vs specialization. Knowing the template args is quite important in > > these cases: > > ``` > > // Primary template isn't deprecated. > > template<typename T> > > struct A {}; > > > > // Specialization is deprecated. > > template<> > > struct [[deprecated("aaa")]] A<int> {}; > > > > // Primary template is deprecated. > > template<typename T> > > struct [[deprecated("bbb")]] B {}; > > > > // Specialization is not deprecated. > > template<> > > struct B<int> {}; > > ``` > > However, I agree that in the case where the primary template is deprecated > > and no specializations differ, the template args don't help all that much. > > > > Also, I don't think we should be so quick to write off consistency. I've > > seen projects parse compiler output before; consistency turns out to be > > important in weird ways. ;-) > > > > > Am I missing something? > > > > I think our definition of "unnecessary" is what differs. I consider the > > template arguments of an instantiation to be necessary as they are part of > > the type definition. Some types in a template set may be deprecated while > > others may not be -- losing the template arguments in *all* cases means > > important information is not conveyed to the user. > > > > If we decide we want to change the way we output template diagnostics in > > the presence of *no* specializations, that's a different discussion. > > However, the code (as it is) is stripping the template arguments in all > > cases. > > My concern is that we elide the template args in *all* cases, not just > > primary vs specialization. > > ... > > However, I agree that in the case where the primary template is deprecated > > and no specializations differ, the template args don't help all that much. > > IIUC, this is the case for all types this check deals with. If it ever > touches template types that are only deprecated for some sets of template > arguments, we should make sure it outputs template arguments, since they > become important. > > > Also, I don't think we should be so quick to write off consistency. I've > > seen projects parse compiler output before; > > I'm pretty sure removing template arguments in this check won't break any > existing projects that parse compiler output ;) > > As for consistency with the -Wdeprecated-declarations diagnostic, I could > have a look at the feasibility of removing template arguments for the cases > where there's no explicit template specialization. > IIUC, this is the case for all types this check deals with. If it ever > touches template types that are only deprecated for some sets of template > arguments, we should make sure it outputs template arguments, since they > become important. That's why I was pushing for this to be a diagnostics engine-level decision -- then we don't have to "remember" to add functionality back in sometime in the future and all checks (including clang) behave consistently without extra intervention. > I'm pretty sure removing template arguments in this check won't break any > existing projects that parse compiler output ;) I'm not as confident as you on this point. ;-) However, I don't think that use case should be an overly strong consideration here, either. https://reviews.llvm.org/D42730 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits