This is an archive of the discontinued LLVM Phabricator instance.

[Concepts] Class template associated constraints
ClosedPublic

Authored by hubert.reinterpretcast on Oct 17 2016, 6:34 AM.

Details

Summary

This adds associated constraints as a property of class templates.
An error is produced if redeclarations are not similarly constrained.

Event Timeline

hubert.reinterpretcast retitled this revision from to [Concepts] Class template associated constraints.
hubert.reinterpretcast updated this object.
test/CXX/concepts-ts/temp/temp.constr/temp.constr.decl/class-template-decl.cpp
13

I should probably add some template-dependent cases.

rsmith added inline comments.Oct 27 2016, 12:06 AM
include/clang/AST/DeclTemplate.h
456–479

Rather than tail-allocating the associated constraints (which will be an inconvenience for every class that derives from TemplateDecl), can you store this as a PointerUnion of a TemplateParameterList* and a pointer to a struct wrapping a TemplateParameterList* and the associated constraints expression? I'm not really concerned about paying one extra pointer per associated constraints expression we store (and I suspect we may want to store this as something richer than an expression at some point).

lib/Sema/SemaTemplate.cpp
1178

I'm not a big fan of continue as a proxy for a goto. It looks like you could rearrange the logic here to avoid this:

bool ConstraintMismatch = false;
if ((bool)CurAC != (bool)PrevAC)
  ConstraintMismatch = true;
else if (CurAC) {
  // profile and set ConstraintMismatch
}
if (ConstraintMismatch) Diag(...)

... or factor out a lambda to do the mismatch checking.

test/CXX/concepts-ts/temp/temp.constr/temp.constr.decl/class-template-decl.cpp
2

I like the idea of separate subdirectories of test/CXX for TSes. Thanks :)

Address review comments; update to revision 292996

Fix possibly ill-formed NDR case

Test template-dependent cases for class redeclaration

Address review comment: use lambda instead of do while(0)

Address review comment: use PointerUnion w/struct

hubert.reinterpretcast marked 3 inline comments as done.Jan 25 2017, 7:25 AM
hubert.reinterpretcast added inline comments.
include/clang/AST/DeclTemplate.h
355

Removed this constructor as a drive-by.

456–479

As discussed in person, I am using PointerUnion in the base class. I am still allocating the proposed struct alongside the TemplateDecl instance where it is not too much trouble.

test/CXX/concepts-ts/temp/temp.constr/temp.constr.decl/class-template-decl.cpp
10

Status of this line is unclear. A conservative user of the language should probably avoid this as ill-formed NDR (functionally equivalent but not equivalent). This line as been replaced.

13

Propagation of associated constraints to instantiated specializations will need to be added later.

hubert.reinterpretcast marked 3 inline comments as done.Jan 25 2017, 3:07 PM
rsmith added inline comments.Feb 8 2017, 3:57 PM
include/clang/AST/DeclTemplate.h
373–391

This mechanism seems unnecessary to me; allocating the ConstrainedTemplateDeclInfo separately seems a lot simpler. Forcing this and the template into a single allocation is unlikely to help anything since we use a slab allocator (which is going to lay the objects out the same way this template trick does, unless we hit the end of a slab).

lib/Sema/SemaTemplate.cpp
1178

[nit] Comments should be full sentences: capitalized and ending in a period.

1299–1300

Is there a reason you don't want to store the associated constraints that were specified on a redeclaration? I'd expect that to hurt tools that want source fidelity (for instance, a renaming tool will want to be able to find all the references to a particular name, even in a requires-clause on a redeclaration of a template).

include/clang/AST/DeclTemplate.h
373–391

Okay; I'll probably allocate separately in the Create function.

lib/Sema/SemaTemplate.cpp
1178

Okay.

1299–1300

Associated constraints are not part of the source fidelity: the requires-clause on the template-parameter-list is (and later, if constraints are introduced by abbreviated function call syntax, etc., the function parameter list).

Address review comments; update to revision 294580

Allocate ConstrainedTemplateDeclInfo separately

Update comments to be sentences; NFC

rsmith accepted this revision.Feb 9 2017, 2:45 PM
rsmith added inline comments.
lib/Sema/SemaTemplate.cpp
1300

Add period to this sentence.

This revision is now accepted and ready to land.Feb 9 2017, 2:45 PM
hubert.reinterpretcast marked 4 inline comments as done.