This is an archive of the discontinued LLVM Phabricator instance.

[Concepts] Associated constraints infrastructure.
ClosedPublic

Authored by saar.raz on Dec 15 2017, 5:38 AM.

Details

Summary

Added code to correctly calculate the associated constraints of a template (no enforcement yet). Depends on D41217.

Diff Detail

Event Timeline

saar.raz created this revision.Dec 15 2017, 5:38 AM
saar.raz retitled this revision from Associated constraint infrastructure. to [Concepts] Associated constraints infrastructure..Dec 15 2017, 5:40 AM
saar.raz edited the summary of this revision. (Show Details)
saar.raz updated this revision to Diff 127163.Dec 15 2017, 11:15 AM
  • Added requires clause matching for all kinds of template redeclarations instead of just classes.
saar.raz updated this revision to Diff 129297.Jan 10 2018, 10:10 AM
  • Fixed wrong if that would cause valid instantiated requires clauses to not be accepted.
saar.raz updated this revision to Diff 132748.Feb 3 2018, 11:55 AM

Moved function into SemaConcept.cpp.

saar.raz updated this revision to Diff 138500.Mar 15 2018, 12:06 AM

Fixed reference to TemplateParams member in assertion.

saar.raz updated this revision to Diff 159236.Aug 5 2018, 3:43 PM
  • Consider requires clause when determining if TPL has an unexpanded pack
rsmith added inline comments.Aug 6 2018, 5:52 PM
lib/AST/DeclTemplate.cpp
203

Rather than producing an Expr* (which will presumably eventually need to contain synthetic && expressions), this should produce a vector of constraints (notionally and'd together), which will (eventually) be either constraint-expressions or some representation of "the constraints implied by this template parameter". If you do that, I don't think there's any reason to cache this information on the TemplateDecl; it should be cheap enough to recompute when we need it.

lib/Sema/SemaConcept.cpp
47–58

I'm sure we already have this "profile two expressions and check they are canonically equivalent" functionality factored out somewhere. Is there nothing like that that's appropriate to reuse here?

lib/Sema/SemaTemplate.cpp
2145–2154

Unrelated cleanup: please commit this separately.

lib/Sema/SemaTemplateInstantiateDecl.cpp
3141–3148

This is wrong (we should never substitute into a constraint-expression except as part of satisfaction or subsumption checks). Please at least add a FIXME.

lib/Serialization/ASTReaderDecl.cpp
2003–2006

There doesn't seem to be a good reason to serialize the cached associated constraints, even if we want to keep them (which I don't think we do).

lib/Serialization/ASTWriter.cpp
5856

What would be left "to do" here? For constrained parameters, there may be something interesting to track on the *Template*ParmDecl, but not here, I wouldn't think,

saar.raz updated this revision to Diff 160861.Aug 15 2018, 11:10 AM
  • Address comments by rsmith, mainly removing associated constraints caching and instead returning a smallvector of constraint expressions from getAssociatedConstraints.
saar.raz updated this revision to Diff 161345.Aug 17 2018, 3:15 PM
  • Fix bad reference to getRequiresClause on TemplateDecl in assertion
Quuxplusone added inline comments.
test/CXX/concepts-ts/temp/temp.constr/temp.constr.decl/var-template-decl.cpp
11

For my own edification, could you explain whether, given

#define BOOL bool
using typedef_for_bool = bool;

you'd expect to diagnose a redeclaration of B::A with associated constraint

requires bool( U() )  // extra whitespace

or

requires BOOL(U())  // different spelling of `bool`

or

requires typedef_for_bool(U())  // different spelling of `bool`

? My naive reading of N4762 temp.constr.atomic/2 says that none of these constraints (on line 10) would be "identical" to the constraint on line 6... but then I don't understand what's the salient difference between line 10 (which apparently gives no error) and line 22 (which apparently gives an error).

saar.raz added inline comments.Aug 18 2018, 2:30 AM
test/CXX/concepts-ts/temp/temp.constr/temp.constr.decl/var-template-decl.cpp
11

Line 22 has a not (!) operator in front of the bool(), I guess you missed that?

test/CXX/concepts-ts/temp/temp.constr/temp.constr.decl/var-template-decl.cpp
11

I saw the !... but I don't understand how the compiler "knows" that !bool(U()) is "different" from bool(T()) in a way that doesn't equally apply to bool(U()).

Or suppose the constraint on line 10 was requires bool(U())==true... would that give a diagnostic?

Rakete1111 added inline comments.
test/CXX/concepts-ts/temp/temp.constr/temp.constr.decl/var-template-decl.cpp
11

bool(T()) and bool(U()) are identical because they have the same parameter mappings.

The "identical" requirement applies to the actual grammar composition of the expression, so bool(T()) would be different to bool(T()) == true.

At least that's how I understand it.

saar.raz added inline comments.Aug 18 2018, 10:09 PM
test/CXX/concepts-ts/temp/temp.constr/temp.constr.decl/var-template-decl.cpp
11

OK, I can see where the confusion is coming from.

The way it works (in clang, at least) - is that the compiler pays no attention to the name of a template parameter for any purpose other than actually finding it in the first place - once it is found, it is 'stored' simply as bool(<template-parameter-0-0>()) where the first 0 is the depth of the template parameter list of the parameter in question (in case of a template within a template) and the second 0 is the index of the template parameter within that list.

I believe this treatment stems from [temp.over.link]p6 "When determining whether types or qualified-concept-names are equivalent, the rules above are used to compare expressions involving template parameters"

saar.raz added inline comments.Aug 18 2018, 10:34 PM
test/CXX/concepts-ts/temp/temp.constr/temp.constr.decl/var-template-decl.cpp
11

Correction - p5 describes this better (see also the example in p4)

test/CXX/concepts-ts/temp/temp.constr/temp.constr.decl/var-template-decl.cpp
11

Okay, I can see how this matches the apparent intent of http://eel.is/c++draft/temp.over.link#5 . I guess a strict reading of that passage would mean that my BOOL and typedef_for_bool versions should give diagnostics, and so should

#define V(x) U
template <typename U> requires bool(V(x) ())

but no diagnostic is expected for

#define V U
template <typename U> requires bool(V ())

Anyway, sounds like this rabbit hole is out-of-scope for this review, anyway, so I'll be quiet now. :)

Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2019, 4:31 PM
saar.raz updated this revision to Diff 194984.Apr 12 2019, 4:56 PM

Rebase onto trunk

rsmith added inline comments.Apr 18 2019, 5:07 PM
include/clang/AST/DeclTemplate.h
178

Returning a SmallVector by value is not idiomatic. If you need to make a copy here, you should take a SmallVectorImpl by reference, so the caller can choose the inline size. If not, you should return an ArrayRef.

include/clang/Sema/Sema.h
5584–5590

This is not how redeclaration checking is specified any more. A redeclaration is valid only if its template-head is equivalent to that of the prior declaration, not if it has the same associated constraints. You should instead take two TemplateParameterLists, profile them, and compare the results.

lib/Serialization/ASTReaderDecl.cpp
2304

There's no such thing any more.

2322

There's no such thing any more.

saar.raz updated this revision to Diff 196074.Apr 22 2019, 8:17 AM
  • Address CR comments by rsmith
    • Remove obsolete TODOs
    • Fix redeclaration checking
    • Change getAssociatedConstraints interface
  • Add requires clause to ASTNodeTraverser
saar.raz updated this revision to Diff 209688.Jul 13 2019, 6:30 AM

Rebase onto trunk.

saar.raz updated this revision to Diff 224898.Oct 14 2019, 1:19 PM
saar.raz marked 15 inline comments as done.

Remove unrelated cleanup

saar.raz marked an inline comment as done.Oct 14 2019, 1:19 PM
rsmith accepted this revision.Oct 14 2019, 3:42 PM
rsmith added inline comments.
clang/include/clang/Sema/Sema.h
5950 ↗(On Diff #224898)

Missing period.

clang/test/CXX/concepts-ts/temp/concept/p4.cpp
1 ↗(On Diff #224898)

Please move this test and the others to the relevant place under test/CXX/temp rather than test/CXX/concepts-ts and remove -fconcepts-ts from the RUN: lines.

This revision is now accepted and ready to land.Oct 14 2019, 3:42 PM
saar.raz closed this revision.Oct 24 2019, 2:37 PM