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:
> > > > > 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.
> > > > That's why I was pushing for this to be a diagnostics engine-level 
> > > > decision
> > > 
> > > Diagnostic engine doesn't have enough information on how to distinguish 
> > > cases where the template parameters in the message are useful and where 
> > > they aren't. Specifically, for the deprecation warnings this depends on 
> > > what is explicitly marked with the [[deprecated]] attribute (primary 
> > > template vs. explicit template instantiation - if this even makes sense 
> > > to do at all). For other warnings the logic will be different. For this 
> > > check there don't seem to be any cases where the template arguments would 
> > > be useful in the message (and I would be surprised, if one of the future 
> > > C++ standards declared only certain instantiations of template classes in 
> > > STL deprecated, but it it does, we can reconsider this decision).
> > > Diagnostic engine doesn't have enough information on how to distinguish 
> > > cases where the template parameters in the message are useful and where 
> > > they aren't. Specifically, for the deprecation warnings this depends on 
> > > what is explicitly marked with the [[deprecated]] attribute (primary 
> > > template vs. explicit template instantiation - if this even makes sense 
> > > to do at all).
> > 
> > By "explicit template instantiation", do you mean explicit specialization? 
> > If so, that definitely makes sense (it's even called out in the standard).
> > 
> > > For other warnings the logic will be different. For this check there 
> > > don't seem to be any cases where the template arguments would be useful 
> > > in the message (and I would be surprised, if one of the future C++ 
> > > standards declared only certain instantiations of template classes in STL 
> > > deprecated, but it it does, we can reconsider this decision).
> > 
> > `std::vector<bool>` immediately comes to mind where it's reasonable to want 
> > to deprecate the specialization and not the primary template (and the C++ 
> > committee might someday decide to do exactly that).
> > By "explicit template instantiation", do you mean explicit specialization? 
> 
> Yes, I meant explicit specialization.
> 
> > If so, that definitely makes sense (it's even called out in the standard).
> 
> Could you clarify? What does the standard say about this?
> 
> > `std::vector<bool>` immediately comes to mind where it's reasonable to want 
> > to deprecate the specialization and not the primary template (and the C++ 
> > committee might someday decide to do exactly that).
> 
> Thanks for the example. vector<bool> is probably the most likely candidate I 
> know so far, though I still find this not extremely likely to happen. And I 
> still don't see how we could accommodate this use case in a generic enough 
> way (in the diagnostic engine, for example). Even this check and the 
> -Wdeprecated-declarations would have to take the decision (of whether the 
> template arguments should be displayed) differently. The diagnostic can rely 
> on the `[[deprecated]]` attribute, while this check would have to do this 
> differently to be able to support language standard(s) older than the one 
> where the deprecation hypothetically took place.
>> By "explicit template instantiation", do you mean explicit specialization?
> Yes, I meant explicit specialization.

Thanks; I kind of thought that, but didn't want to assume for too long. :-)

>> If so, that definitely makes sense (it's even called out in the standard).
> Could you clarify? What does the standard say about this?

I was responding to the comment "if this even makes sense to do at all" with a 
rationale as to why I think it makes sense:

[dcl.attr.deprecated]p2: "The attribute may be applied to the declaration of a 
class, a typedef-name, a variable, a non-static data member, a function, a 
namespace, an enumeration, an enumerator, or a template specialization."

The key bit is that it may be applied to a template specialization and 
differently than the primary template.

> Thanks for the example. vector<bool> is probably the most likely candidate I 
> know so far, though I still find this not extremely likely to happen.

I'm less worried about what WG21 does there (though I do think we may someday 
get to the point where we can deprecated vector<bool>) and more worried about 
people who want to flag it *locally*, but I see now that there's no 
configuration options yet, so perhaps that's moot.

>  And I still don't see how we could accommodate this use case in a generic 
> enough way (in the diagnostic engine, for example). 

I was thinking of using different qualifiers on the diagnostic placeholder, 
kind of like what we do with %q to force it to always print a qualified name. I 
was thinking we could use a separate modifier to say "this particular check 
allows you to do some other heuristic for printing template argument lists", 
such as only providing the template arguments in the presence of explicit 
specializations. 

If we plan to never allow this to be customizable, then I suppose we can 
address the template args issue when it arises with standards-based classes. 
However, I remain skeptical that this is a good precedent to set.


https://reviews.llvm.org/D42730



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to