This is an archive of the discontinued LLVM Phabricator instance.

Variable templates w/ partial support for static data members
ClosedPublic

Authored by lvoufo on Jun 28 2013, 5:30 PM.

Details

Reviewers
rsmith
Summary

This patch should cover all aspects of top-level variable template declarations, including explicit instantiations and (partial) specializations.
I have also incorporated comments from the first patch.
I am still completing test cases, but thought I'd send this out for the time being, to catch earlier misses or get a better sense of what other test cases to cover.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
rsmith added inline comments.Jul 9 2013, 6:26 PM
include/clang/AST/Decl.h
1150

Indentation.

include/clang/AST/DeclTemplate.h
2285

Missing period.

2310

Missing period.

2318–2326

Indentation.

2443

"members of the variable template"? Maybe "the initializer of the variable template"?

2484

Last sentence here seems incorrect; drop it?

2545

"*" on the right, please.

2552–2555

Is this necessary, given that we store specializations in a FoldingSetVector (which maintains order itself)?

2565–2578

Indentation.

2596

Indentation.

2660–2661

Extra "template" at the start of the second line.

2691

Isn't this handled by the Profile support in the base class?

2712–2713

... for this *variable* template

2716–2717

... for this *variable* template

2742

Indentation.

2756

... underlying variable declaration

2762

... primary variable template pattern.

2769–2773

Indentation.

2836–2844

This doesn't make sense for a variable template; just remove it.

include/clang/AST/RecursiveASTVisitor.h
1484–1532

Once this patch lands, it'd be good to go back over this and clean up this duplication: traversal for variable templates, class templates, and function templates all use exactly the same code (other than some trivial differences in class names).

1886

Grammar.

include/clang/Basic/DiagnosticSemaKinds.td
2843

We usually word this as "variable templates are a C++1y extension". Please also add a CXXPre1yCompat warning, "variable templates are incompatible with C++ standards before C++1y". (The idea is to allow people to compile with -std=c++1y, and get warned if their code would not be portable to earlier standard versions.)

3113

Indentation.

3114–3115

Should this say "... cannot be redefined" ? The corresponding rule for class template partial specializations in 14.5.5/5 allows redeclarations.

3119

Indentation.

3274–3276

Where's %1?

include/clang/Sema/Sema.h
1386

"||" on previous line, please.

1391–1399

Remove this assert; it's covered by your default case below.

5033

Capital I.

6373–6374

"*" on the right, please

include/clang/Sema/Template.h
385

Indentation

451

Indentation (and probably unnecessary line wrapping)

499–501

Reformat this, please

lib/AST/ASTContext.cpp
1062

Accidental change?

lib/AST/ASTImporter.cpp
106

Updates to ASTImporter.cpp aren't generally necessary: it's already *very* incomplete (it doesn't support function templates, for instance) and would require substantial work to make it complete.

2038

I think this FIXME fits better in the caller.

4175

We don't support variable templates declared at function scope, right?

4236–4237

Did you intend to reuse ImportDefinition here?

4318

class?

4327

function?

lib/AST/Decl.cpp
1563

You're not using PrevDecl here; remove it?

2000–2002

Indentation

2009–2011

Indentation.

lib/AST/DeclTemplate.cpp
971–976

Indentation.

983

Indentation (and likewise later functions in this file).

1208–1209

Maybe adopt them into DC (though they should presumably be there already).

lib/Parse/Parser.cpp
1431

Update this comment.

1646–1649

No linebreak here.

lib/Sema/SemaDecl.cpp
4833–4839

"Is", not "is", please.

4836

It seems error-prone to have this in scope for the entire function but only use it when declaring a variable template.

4914–4918

Don't bail out here, just accept the variable template as if we were in C++1y mode.

4931

Indentation.

4939

What happens in the "else" case of this? (template<> int n;)

4980

Extra space after paren.

4987–4990

Extra spaces after ( here.

5017–5018

No linebreak here.

5033–5041

Doesn't look like it; it looks incorrect for variable templates and redundant otherwise.

5231–5232

Should do this regardless of language mode.

5249

It might be clearer to set this on NewTemplate (even though they share a CommonPtr).

lib/Sema/SemaExpr.cpp
1541–1542

Remove this.

1553–1559

Reindent. Also, assert(!TemplateArgs) here?

2036

No C++1y check here; we're supporting variable templates in earlier languages as an extension.

2045–2046

Nope.

11184–11185

Remove comment: no need for a special case here; variable template specializations never have automatic storage duration so aren't candidates to be captured anyway.

11571

Did you mean to leave this in the patch?

11603–11604

Remove this.

11622–11623

Should this check be outside the usable-in-constant-expressions check?

11641–11648

Revert these (non-)changes.

lib/Sema/SemaTemplate.cpp
2281–2283

Indentation.

2293

Drop this comment, it's not really useful to future readers of the code.

2300

"Is", not "is"

2303

"D must be a template-id" ?

2354

Space after "if".

2388–2395

Indentation.

2407

No parens here.

2440

...of a member variable

2443

As before, this might be clearer if you call setMemberSpecialization on Partial.

2445

...of the variable template

2447

...this variable template...

2524

Missing space after 'if'.

2552–2553

Remove.

2559

"*" on the right.

2567

Indentation.

2591

What happens when the type of the variable depends on the initializer:

template<typename T> auto var1 = T();
template<int...N> int var2[] = { N... };
2591–2594

Indentation.

2617–2660

Please factor the common code out of TemplateDeclInstantiator::VisitVarDecl and here.

2668

Please do. It looks like it should be straightforward to extract this as a template parameterized on the relevant type of partial specialization decl.

2781

"// Build"

2807–2808

Remove.

7260–7261

Remove.

7277

Is this const_cast necessary?

7278

Space after 'if'.

7289

Space after 'if'

7297–7308

No linebreak here.

7330

Why check the scope specifier here? It looks like this won't reject:

template<typename T> T var = T();
template int var;

Do we handle that elsewhere?

Please also update the preceding quotation based on N3690 (the new wording includes some relevant text about variable templates.)

7341

This assert is dead. Maybe change the "else if (PrevTemplate) {" below to "else { assert(PrevTemplate && ...)" ?

7347

Space after 'if'

lib/Sema/SemaTemplateDeduction.cpp
2412

...given variable template

4477–4488

s/class/variable/g

4497–4498

Remove?

rsmith added inline comments.Jul 9 2013, 6:26 PM
lib/Sema/SemaTemplateDeduction.cpp
4489–4550

Instead of duplicating it, can you template the existing getMoreSpecializedPartialSpecialization on the type of PartialSpecializationDecl?

lib/Sema/SemaTemplateInstantiateDecl.cpp
324–325

"In C++11"?

330–331

Remove.

336

Indentation / join with previous line.

389

No test for C++1y here.

490–491

No linebreak here.

1090–1092

Remove.

1114–1115

Add a FIXME here to revisit this.

1219

Can this happen? Should we assert here?

1223–1224

Likewise.

2789

Remove?

3427

Can this happen?

3429–3431

Should we call ActOnInitializerError here?

3439–3445

Why commented out?

3494

"*" on the right. Also, maybe inline the call into the 'if', since you're not otherwise using the variable.

3514

Indentation.

3540

Capital P.

3546

Space after 'if'

3885–3898

Remove?

4155

Space after 'if'.

4155–4157

Use

if (VarTemplateSpecializationDecl *VarSpec = dyn_cast<VarTemplateSpecializationDecl>(D)) {

Rather than isa + cast.

4161–4163

No braces.

4348–4349

Remove.

lib/Sema/TreeTransform.h
6205

Nothing changed in this file but whitespace; revert?

lib/Serialization/ASTWriterDecl.cpp
1195–1207

s/CT/VT/g

1211–1212

Remove

1217–1271

It looks like the corresponding code in ASTDeclReader is missing?

lvoufo added inline comments.Jul 30 2013, 1:38 PM
include/clang/AST/Decl.h
1205

Right. I've removed PrevDecl from VarDecl.

include/clang/AST/RecursiveASTVisitor.h
1484–1532

Right. I've added a FIXME to this effect.

lib/AST/ASTImporter.cpp
106

So... Is this an invitation to drop these changes, or to keep them around at least for future-proofness reasons?

lib/AST/Decl.cpp
1563

Ow. Good catch.

lib/AST/DeclTemplate.cpp
1208–1209

I think they should be in DC by now. I leave this commented out for now, and remove it once I'm sure that there are no unforeseen corner case.

lib/Sema/SemaExpr.cpp
11571

Yes. But I don't find it necessary anymore.

lib/Sema/SemaTemplate.cpp
2591

Added a FIXME for now.

7330

It parses "template int var" as an explicit instantiation and complains about "too few arguments".

lib/Sema/SemaTemplateDeduction.cpp
4489–4550

Marked as TODO.

lib/Sema/SemaTemplateInstantiateDecl.cpp
1114–1115

Removed.

3439–3445

I had meant to remove it. This block of code is only used in TemplateDeclInstantiator::VisitVarDecl()

lib/Serialization/ASTWriterDecl.cpp
1217–1271

Right. I had missed that. Thanks.

lvoufo updated this revision to Unknown Object (????).Aug 2 2013, 3:29 PM

This is an update from the previous patch with some-but-not-all support for static data member templates.

Comments from previous reviews have also been incorporated here.
Missing features are documented as FIXMEs and TODOs.

More test cases, including as requested in the reviews, are under development.

lvoufo updated this revision to Unknown Object (????).Aug 2 2013, 7:01 PM

auto variable templates should now behave according to the standard.

Next up:

  • proper diagnosis for redefinitions (see notes in test file)
  • complete static data member templates coverage
  • more elaborate test cases
lvoufo updated this revision to Unknown Object (????).Aug 3 2013, 6:44 PM

Cleaned up the tests cases a little bit, reviewing diagnosis for redefinitions of variable templates and variable template specializations.

Some issues with static data members seem to have resolved themselves as well. However, there are still two important issues, identified in the test files, that need be addressed before working on further test cases:

  1. const pointer-typed static data member templates seem mis-diagnosed.
  2. The explicit specialization and instantiation of static data member templates of class templates leads to runtime exceptions.

I will be inserting more test cases as indicated in the previous reviews, while treating the above issues as more of a priority. Please let me know if you disagree...

lvoufo updated this revision to Unknown Object (????).Aug 4 2013, 11:55 AM

The runtime exceptions generated by static data member templates of class templates have been resolved via a somewhat ugly hack.
Related type substitution is still not working properly. Need to revise TemplateDeclInstantiator's VisitVarTemplateDEcl() and VisitVarTemplatePartialSpecializationDecl() to this effect...

lvoufo updated this revision to Unknown Object (????).Aug 4 2013, 3:37 PM

Updated test cases, showing variable templates working even when C++1y is not enabled. appropriate warnings are generated, also indicating feature compatibility.

Of all the test cases requested in previous review, the following constitute the main remaining ones:

  • Tests for PCH (serialization/deserialization)
  • Tests for name mangling of variable templates, and IR generation generally (for instance, implicit internal linkage of variable template that instantiates to a const-qualified type).

All other tests cases will be updated as the feature is completed.

rsmith added inline comments.Aug 4 2013, 11:29 PM
include/clang/AST/ASTContext.h
275

Typo "synonym"

include/clang/AST/Decl.h
954

No parens around these.

include/clang/AST/DeclTemplate.h
2402–2403

Extra newline in this comment.

2436–2443

Likewise.

2620–2621

And here.

include/clang/AST/RecursiveASTVisitor.h
1535–1537

Join these lines.

1902–1903

Join these lines.

include/clang/Basic/DiagnosticSemaKinds.td
3066–3068

It looks like you don't use this diagnostic; remove.

3110

This has two spaces in a row if %0 is 0.

include/clang/Sema/Template.h
381–386

You don't appear to use these for anything yet; I assume support for class-scope partial specialization of variable templates will follow in a separate patch?

lib/AST/ASTContext.cpp
1044–1049

It looks like this has no callers; remove it.

7995–7996

Why is this commented out?

lib/AST/Decl.cpp
1987

Remove commented out code.

lib/Parse/ParseCXXInlineMethods.cpp
59–62 ↗(On Diff #3182)

This can't happen: FnD is always a function declaration here. (We only handle initializers here in order to deal with a "= 0" pure-specifier.)

lib/Parse/ParseDecl.cpp
1805–1816

This check seems like it belongs in Sema.

lib/Parse/ParseExprCXX.cpp
564–569 ↗(On Diff #3182)

This should be handled elsewhere: perhaps in ParseOptionalCXXScopeSpecifier's handling of annot_template_id.

lib/Sema/SemaDecl.cpp
4945
if (TemplateParams->empty() &&
4962

... partial or explicit specialization.

5018–5019

Redundant newline.

lib/Sema/SemaTemplate.cpp
216

Remove.

2583

*do not

lib/Sema/SemaTemplateDeduction.cpp
2404

... given variable template

lib/Sema/SemaTemplateInstantiateDecl.cpp
2313–2315

Is it really valid for a type to be defined in a variable template declaration? That seems like a big can of worms.

rsmith added inline comments.Aug 4 2013, 11:35 PM
test/SemaCXX/cxx1y-variable-templates_in_class.cpp
161–169 ↗(On Diff #3188)

This is going to be a maintenance nightmare if we ever change these tests. Please instead turn the relevant warnings off for this test, and add a test for this warning to test/SemaCXX/cxx98-compat.cpp

test/SemaCXX/cxx1y-variable-templates_top_level.cpp
403–413 ↗(On Diff #3188)

Put these next to the corresponding parts of the test, and use relative line numbers not absolute, to make the test less fragile.

416–419 ↗(On Diff #3188)

Likewise please turn this warning off.

lvoufo added inline comments.Aug 5 2013, 7:53 AM
test/SemaCXX/cxx1y-variable-templates_in_class.cpp
161–169 ↗(On Diff #3188)

Yes. I was a little bit worried about this too. But I wasn't sure how to balance out thoroughness against maintenance...

I will change this in the next update.

test/SemaCXX/cxx1y-variable-templates_top_level.cpp
403–413 ↗(On Diff #3188)

Ok.

416–419 ↗(On Diff #3188)

Ok.

lvoufo added inline comments.Aug 5 2013, 8:37 AM
include/clang/AST/RecursiveASTVisitor.h
1902–1903

Yeah. I need to be a bit more attentive with clang-format.

include/clang/Basic/DiagnosticSemaKinds.td
3066–3068

Right. This must have been lost with previous revisions.

3110

Fixed in one of the lasts patches after this...

include/clang/Sema/Template.h
381–386

Probably. We have some class-scope partial specialization right now. However, it looks like I have not well mapped the behavior from nested templates. So, I guess I'll know for sure after that is done.

lib/AST/ASTContext.cpp
1044–1049

I have replaced it with getTemplateOrSpecializationInfo(). However, I'd like to make sure that static data member templates are working well before I remove it completely...

7995–7996

I was in the process of replacing it with TemplateOrInstantiation. Removed.

lib/AST/Decl.cpp
1987

Same logic as earlier. I am not 100% confident that it is not needed yet. I will eventually removed it when/if I get static data member templates to work properly. I added a FIXME for now.

lib/Parse/ParseCXXInlineMethods.cpp
59–62 ↗(On Diff #3182)

Ok.

lib/Parse/ParseDecl.cpp
1805–1816

Ok.

lib/Parse/ParseExprCXX.cpp
564–569 ↗(On Diff #3182)

Ok. Noted.

lib/Sema/SemaTemplateInstantiateDecl.cpp
2313–2315

I'm not sure. But perhaps this is another question to add onto my "notes".

I interpret this as allowing things like

template<typename T> class { } var = ...;

since "class { }" is anonymous and this is okay:

class {} var;

But that can lead to things like

template<typename T> class {T foo;} var = ...;

Which can be "tricky" to manage...

Should we just reject all such cases, or at least just allow the first one, where the anonymous type is non-dependent?

lvoufo added inline comments.Aug 5 2013, 4:15 PM
lib/Sema/SemaDecl.cpp
4945

Small note. There is no "empty()" in TemplateParameterList.

lvoufo updated this revision to Unknown Object (????).Aug 5 2013, 4:21 PM

Reviews incorporated. Ready to push?

rsmith accepted this revision.Aug 5 2013, 5:04 PM

OK, please go ahead and commit this, all the FIXMEs look fine to handle in subsequent commits. Many thanks!

test/SemaCXX/cxx1y-variable-templates_in_class.cpp
1–2 ↗(On Diff #3204)

Please use -Wno-c++11-extensions -Wno-c++1y-extensions here, rather than -w.

test/SemaCXX/cxx1y-variable-templates_top_level.cpp
1–2 ↗(On Diff #3204)

Likewise.

Eugene.Zelenko closed this revision.Oct 5 2016, 1:17 PM
Eugene.Zelenko added a subscriber: Eugene.Zelenko.

Committed in rL187762.