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