This is an archive of the discontinued LLVM Phabricator instance.

Correct class-template deprecation behavior-REDUX
ClosedPublic

Authored by erichkeane on Mar 22 2017, 9:57 AM.

Details

Summary

Correct class-template deprecation behavior

Based on the comment in the test, and my reading of the standard, a deprecated warning should be issued in the following case:
template<typename T> [[deprecated]] class Foo{}; Foo<int> f;

This was not the case, because the ClassTemplateSpecializationDecl creation did not also copy the deprecated attribute.

Note: I did NOT audit the complete set of attributes to see WHICH ones should be copied, so instead I simply copy ONLY the deprecated attribute.

Previous Differential Revision: https://reviews.llvm.org/D27486, was reverted.
This patch fixes the issues brought up here by the reverter: https://reviews.llvm.org/rL298410

Diff Detail

Repository
rL LLVM

Event Timeline

erichkeane created this revision.Mar 22 2017, 9:57 AM
mboehme edited edge metadata.Mar 22 2017, 10:02 AM

Spelling: Filename template-multiple-attr-propegation.cpp should be "...propagation.cpp"

Fix spelling of propagation in the test name.

aaron.ballman added inline comments.Mar 23 2017, 9:32 AM
lib/Sema/SemaDeclAttr.cpp
6749 ↗(On Diff #92652)

Can be const auto *.

lib/Sema/SemaTemplate.cpp
2849 ↗(On Diff #92652)

Extra space before the ==.

lib/Sema/SemaTemplateInstantiateDecl.cpp
331 ↗(On Diff #92652)

The * should bind to the identifier not the type.

336 ↗(On Diff #92652)

Use of an rvalue reference seems suspicious here. I think this should be `const auto *' rather than an rvalue reference to a pointer.

Better yet, use llvm::find_if().

erichkeane added inline comments.Mar 23 2017, 9:36 AM
lib/Sema/SemaTemplateInstantiateDecl.cpp
336 ↗(On Diff #92652)

This is actually a universal reference, generally a template code idiom (since it does the 'right thing' for all types) that seems to have escaped. I was unaware with 'find_if', so I'll likely switch to that anyway though!

aaron.ballman added inline comments.Mar 23 2017, 9:42 AM
lib/Sema/SemaTemplateInstantiateDecl.cpp
336 ↗(On Diff #92652)

This isn't a universal reference -- the type being deduced is not a cv-unqualified template parameter. See [temp.deduct.call]p3 for details.

erichkeane added inline comments.Mar 23 2017, 10:12 AM
lib/Sema/SemaTemplateInstantiateDecl.cpp
336 ↗(On Diff #92652)

I can't find the standardese, but my understanding was/is that '&&' is a universal reference in any deduced context. Since it is using 'auto', which is always deduced, 'auto &&' should be a universal reference. I DO note that it doesn't match the definition of forwarding reference in temp.deduct.call, so I'll have to poke around a bit more.

I'll note that in looking around, I found this: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4164.pdf where answer 3.3 claims that auto&& for local variables is still a forwarding reference, though it is sadly shy of standardese quotes.

That said, I'll get back to fixing this patch :)

aaron.ballman added inline comments.Mar 23 2017, 10:24 AM
lib/Sema/SemaTemplateInstantiateDecl.cpp
336 ↗(On Diff #92652)

Hmm, I think I just needed to read the standard a bit harder to remember that auto type deduction uses the template argument deduction rules. You're correct, that is a universal reference in that case (sorry for the confusion).

The interesting bits, btw, are: [dcl.type.auto.deduct]p4 and [temp.deduct.call]p3.

BTW, patch incoming!

lib/Sema/SemaTemplateInstantiateDecl.cpp
336 ↗(On Diff #92652)

Ah! [dcl.type.auto.deduct]p4 is exactly what I was looking for! Sadly, my knowledge of the standard is pretty immature, so I appreciated the opportunity to read more into it.

Changes Aaron suggested

aaron.ballman added inline comments.Mar 23 2017, 11:06 AM
lib/Sema/SemaTemplateInstantiateDecl.cpp
331 ↗(On Diff #92830)

Sorry to have missed this one earlier, but that should be taking a const Decl * and const Attr *. Also, the function should be marked static.

334 ↗(On Diff #92830)

const Attr * here as well. Also, feel free to use & if you don't want to capture explicitly.

Being better about const, I prefer the strictest capture, so unless we have a policy different, I'll leave it.

aaron.ballman accepted this revision.Mar 23 2017, 11:19 AM

Being better about const, I prefer the strictest capture, so unless we have a policy different, I'll leave it.

Nope, no policy issue, only the usual readability one (which this is fine for). I found one grammatical issue in a comment (you can fix when you commit), but otherwise LGTM!

include/clang/Basic/Attr.td
305 ↗(On Diff #92836)

attribute meaningful -> attribute is meaningful

This revision is now accepted and ready to land.Mar 23 2017, 11:19 AM
This revision was automatically updated to reflect the committed changes.