[PATCH] D40381: Parse concept definition
saar.raz updated this revision to Diff 209068.
saar.raz added a comment.
Final committed diff.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D40381/new/
https://reviews.llvm.org/D40381
Files:
include/clang/AST/ASTNodeTraverser.h
include/clang/AST/DeclTemplate.h
include/clang/AST/RecursiveASTVisitor.h
include/clang/AST/TextNodeDumper.h
include/clang/Basic/DeclNodes.td
include/clang/Basic/DiagnosticParseKinds.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/TemplateKinds.h
include/clang/Parse/Parser.h
include/clang/Sema/Sema.h
include/clang/Serialization/ASTBitCodes.h
lib/AST/ASTStructuralEquivalence.cpp
lib/AST/DeclBase.cpp
lib/AST/DeclPrinter.cpp
lib/AST/DeclTemplate.cpp
lib/AST/TextNodeDumper.cpp
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CodeGenModule.cpp
lib/Index/IndexDecl.cpp
lib/Parse/ParseTemplate.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaDeclCXX.cpp
lib/Sema/SemaLookup.cpp
lib/Sema/SemaTemplate.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
lib/Serialization/ASTCommon.cpp
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriter.cpp
lib/Serialization/ASTWriterDecl.cpp
test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp
test/Parser/cxx-concept-declaration.cpp
test/Parser/cxx-concepts-ambig-constraint-expr.cpp
test/Parser/cxx-concepts-requires-clause.cpp
test/Parser/cxx2a-concept-declaration.cpp
test/Parser/cxx2a-concepts-ambig-constraint-expr.cpp
test/Parser/cxx2a-concepts-requires-clause.cpp
tools/libclang/CIndex.cpp
Index: tools/libclang/CIndex.cpp
===
--- tools/libclang/CIndex.cpp
+++ tools/libclang/CIndex.cpp
@@ -6269,6 +6269,7 @@
case Decl::PragmaComment:
case Decl::PragmaDetectMismatch:
case Decl::UsingPack:
+ case Decl::Concept:
return C;
// Declaration kinds that don't make any sense here, but are
Index: test/Parser/cxx2a-concept-declaration.cpp
===
--- /dev/null
+++ test/Parser/cxx2a-concept-declaration.cpp
@@ -0,0 +1,73 @@
+// Support parsing of concepts
+
+// RUN: %clang_cc1 -std=c++2a -fconcepts-ts -verify %s
+template concept C1 = true; // expected-note 2{{previous}}
+
+template concept C1 = true; // expected-error{{redefinition}}
+
+template concept D1 = true;
+// expected-error@-1{{expected template parameter}}
+// expected-error@-2{{concept template parameter list must have at least one parameter; explicit specialization of concepts is not allowed}}
+
+template concept T> concept D2 = true;
+// expected-error@-1{{expected identifier}}
+// expected-error@-2{{template template parameter requires 'class' after the parameter list}}
+// expected-error@-3{{concept template parameter list must have at least one parameter; explicit specialization of concepts is not allowed}}
+
+template concept C2 = 0.f; // expected-error {{constraint expression must be of type 'bool' but is of type 'float'}}
+
+struct S1 {
+ template concept C1 = true; // expected-error {{concept declarations may only appear in global or namespace scope}}
+};
+
+extern "C++" {
+ template concept C1 = true; // expected-error{{redefinition}}
+}
+
+template
+template
+concept C4 = true; // expected-error {{extraneous template parameter list in concept definition}}
+
+template concept C5 = true; // expected-note {{previous}} expected-note {{previous}}
+int C5; // expected-error {{redefinition}}
+struct C5 {}; // expected-error {{redefinition}}
+
+struct C6 {}; // expected-note{{previous definition is here}}
+template concept C6 = true;
+// expected-error@-1{{redefinition of 'C6' as different kind of symbol}}
+
+// TODO: Add test to prevent explicit specialization, partial specialization
+// and explicit instantiation of concepts.
+
+template
+struct integral_constant { static constexpr T value = v; };
+
+namespace N {
+ template concept C7 = true;
+}
+using N::C7;
+
+template concept C8 = integral_constant::value;
+// expected-error@-1{{use of undeclared identifier 'wor'; did you mean 'word'?}}
+// expected-note@-2{{'word' declared here}}
+
+template concept bool C9 = true;
+// expected-warning@-1{{ISO C++2a does not permit the 'bool' keyword after 'concept'}}
+
+template<> concept C10 = false;
+// expected-error@-1{{concept template parameter list must have at least one parameter; explicit specialization of concepts is not allowed}}
+
+template<> concept C9 = false;
+// expected-error@-1{{name defined in concept definition must be an identifier}}
+
+template concept N::C11 = false;
+// expected-error@-1{{name defined in concept definition must be an identifier}}
+
+class A { };
+// expected-note@-1{{'A' declared here}}
+
+template concept A::C12 = false;
+// expected-error@-1{{expected namespace name}}
+
+template concept operator int = false;
+// expected-error@-1{{name defined in concept definition must be an identifier}}
Index: test/Parser/cxx-concept-declaration.cpp
[PATCH] D40381: Parse concept definition
saar.raz added a comment. @rsmith? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D40381/new/ https://reviews.llvm.org/D40381 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
saar.raz added a comment. Awesome! I do not have commit permissions though, so can you do the actual commit? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D40381/new/ https://reviews.llvm.org/D40381 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
saar.raz updated this revision to Diff 198200.
saar.raz marked 4 inline comments as done.
saar.raz added a comment.
- Address final CR comments by rsmith
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D40381/new/
https://reviews.llvm.org/D40381
Files:
include/clang/AST/ASTNodeTraverser.h
include/clang/AST/DeclTemplate.h
include/clang/AST/RecursiveASTVisitor.h
include/clang/AST/TextNodeDumper.h
include/clang/Basic/DeclNodes.td
include/clang/Basic/DiagnosticParseKinds.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/TemplateKinds.h
include/clang/Parse/Parser.h
include/clang/Sema/Sema.h
include/clang/Serialization/ASTBitCodes.h
lib/AST/ASTStructuralEquivalence.cpp
lib/AST/DeclBase.cpp
lib/AST/DeclPrinter.cpp
lib/AST/DeclTemplate.cpp
lib/AST/TextNodeDumper.cpp
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CodeGenModule.cpp
lib/Index/IndexDecl.cpp
lib/Parse/ParseTemplate.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaDeclCXX.cpp
lib/Sema/SemaLookup.cpp
lib/Sema/SemaTemplate.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
lib/Serialization/ASTCommon.cpp
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriter.cpp
lib/Serialization/ASTWriterDecl.cpp
test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp
test/Parser/cxx-concept-declaration.cpp
test/Parser/cxx-concepts-ambig-constraint-expr.cpp
test/Parser/cxx-concepts-requires-clause.cpp
test/Parser/cxx2a-concept-declaration.cpp
test/Parser/cxx2a-concepts-ambig-constraint-expr.cpp
test/Parser/cxx2a-concepts-requires-clause.cpp
tools/libclang/CIndex.cpp
Index: tools/libclang/CIndex.cpp
===
--- tools/libclang/CIndex.cpp
+++ tools/libclang/CIndex.cpp
@@ -6252,6 +6252,7 @@
case Decl::PragmaComment:
case Decl::PragmaDetectMismatch:
case Decl::UsingPack:
+ case Decl::Concept:
return C;
// Declaration kinds that don't make any sense here, but are
Index: test/Parser/cxx-concepts-requires-clause.cpp
===
--- /dev/null
+++ test/Parser/cxx-concepts-requires-clause.cpp
@@ -1,82 +0,0 @@
-// RUN: %clang_cc1 -std=c++14 -fconcepts-ts -x c++ %s -verify
-// expected-no-diagnostics
-
-// Test parsing of the optional requires-clause in a template-declaration.
-
-template requires true
-void foo() { }
-
-
-template requires !0
-struct A {
- void foo();
- struct AA;
- enum E : int;
- static int x;
-
- template requires true
- void Mfoo();
-
- template requires true
- struct M;
-
- template requires true
- static int Mx;
-
- template requires true
- using MQ = M;
-};
-
-template requires !0
-void A::foo() { }
-
-template requires !0
-struct A::AA { };
-
-template requires !0
-enum A::E : int { E0 };
-
-template requires !0
-int A::x = 0;
-
-template requires !0
-template requires true
-void A::Mfoo() { }
-
-template requires !0
-template requires true
-struct A::M { };
-
-template requires !0
-template requires true
-int A::Mx = 0;
-
-
-template requires true
-int x = 0;
-
-template requires true
-using Q = A;
-
-struct C {
- template requires true
- void Mfoo();
-
- template requires true
- struct M;
-
- template requires true
- static int Mx;
-
- template requires true
- using MQ = M;
-};
-
-template requires true
-void C::Mfoo() { }
-
-template requires true
-struct C::M { };
-
-template requires true
-int C::Mx = 0;
Index: test/Parser/cxx-concepts-ambig-constraint-expr.cpp
===
--- /dev/null
+++ test/Parser/cxx-concepts-ambig-constraint-expr.cpp
@@ -1,29 +0,0 @@
-// RUN: %clang_cc1 -std=c++14 -fconcepts-ts -x c++ %s -verify
-
-// Test parsing of constraint-expressions in cases where the grammar is
-// ambiguous with the expectation that the longest token sequence which matches
-// the syntax is consumed without backtracking.
-
-// type-specifier-seq in conversion-type-id
-template requires (bool)::operator short
-unsigned int foo(); // expected-error {{C++ requires a type specifier for all declarations}}
-
-// type-specifier-seq in new-type-id
-template requires (bool)sizeof new (T::f()) short
-unsigned int bar(); // expected-error {{C++ requires a type specifier for all declarations}}
-
-template requires (bool)sizeof new (T::f()) unsigned // expected-error {{'struct' cannot be signed or unsigned}}
-struct X { }; // expected-error {{'X' cannot be defined in a type specifier}}
-
-// C-style cast
-// of function call on function-style cast
-template requires (bool(T()))
-T (*fp)(); // expected-error {{use of undeclared identifier 'fp'}}
-
-// function-style cast
-// as the callee in a function call
-struct A {
- static int t;
- template requires bool(T())
- (A(T ())) { } // expected-error {{called object type 'bool' is not a function or function pointer}}
-};
Index: test/Parser/cxx2a-concept-declaration.cpp
[PATCH] D40381: Parse concept definition
rsmith accepted this revision.
rsmith added a comment.
LGTM with a few mechanical updates.
Comment at: include/clang/Basic/DiagnosticParseKinds.td:1262
+ "name defined in concept definition must be an identifier">;
+def err_concept_legacy_bool_keyword : ExtWarn<
+ "ISO C++2a does not permit the 'bool' keyword after 'concept'">,
`ExtWarn` diagnostics should be named `ext_` not `err_`. (This is important
because readers of the code emitting the diagnostic need to know whether they
can mark things as invalid (etc) during error recovery -- which is only correct
to do after emitting an error.)
Comment at: include/clang/Sema/Sema.h:6676
+ // Concepts
+ Decl *ActOnConceptDefinition(Scope *S,
+ MultiTemplateParamsArg TemplateParameterLists, IdentifierInfo *Name,
Nit: please wrap the first parameter onto the next line.
Comment at: lib/AST/DeclTemplate.cpp:833-834
+ Expr *ConstraintExpr) {
+ // TODO: Do we need this?
+ // AdoptTemplateParameterList(Params, cast(Decl));
+ return new (C, DC) ConceptDecl(DC, L, Name, Params, ConstraintExpr);
Yes, you need that :)
(You should be able to check this with `-ast-dump`: for a concept in a
namespace, its template parameters should have the namespace as their semantic
`DeclContext`, not the translation unit. This also has some impact on merging
of default argument visibility with modules.)
Comment at: lib/Index/IndexDecl.cpp:677
if (D->getTemplateParameters() &&
-shouldIndexTemplateParameterDefaultValue(Parent)) {
+shouldIndexTemplateParameterDefaultValue(D, Parent)) {
const TemplateParameterList *Params = D->getTemplateParameters();
Please revert the addition of the unused first parameter.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D40381/new/
https://reviews.llvm.org/D40381
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
saar.raz added a comment. @rsmith are we done with CR on this? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D40381/new/ https://reviews.llvm.org/D40381 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
saar.raz updated this revision to Diff 196009.
saar.raz added a comment.
Herald added a reviewer: martong.
Address CR comments by rsmith
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D40381/new/
https://reviews.llvm.org/D40381
Files:
include/clang/AST/ASTNodeTraverser.h
include/clang/AST/DeclTemplate.h
include/clang/AST/RecursiveASTVisitor.h
include/clang/AST/TextNodeDumper.h
include/clang/Basic/DeclNodes.td
include/clang/Basic/DiagnosticParseKinds.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/TemplateKinds.h
include/clang/Parse/Parser.h
include/clang/Sema/Sema.h
include/clang/Serialization/ASTBitCodes.h
lib/AST/ASTStructuralEquivalence.cpp
lib/AST/DeclBase.cpp
lib/AST/DeclPrinter.cpp
lib/AST/DeclTemplate.cpp
lib/AST/TextNodeDumper.cpp
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CodeGenModule.cpp
lib/Index/IndexDecl.cpp
lib/Parse/ParseTemplate.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaDeclCXX.cpp
lib/Sema/SemaLookup.cpp
lib/Sema/SemaTemplate.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
lib/Serialization/ASTCommon.cpp
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriter.cpp
lib/Serialization/ASTWriterDecl.cpp
test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp
test/Parser/cxx-concept-declaration.cpp
test/Parser/cxx-concepts-ambig-constraint-expr.cpp
test/Parser/cxx-concepts-requires-clause.cpp
test/Parser/cxx2a-concept-declaration.cpp
test/Parser/cxx2a-concepts-ambig-constraint-expr.cpp
test/Parser/cxx2a-concepts-requires-clause.cpp
tools/libclang/CIndex.cpp
Index: tools/libclang/CIndex.cpp
===
--- tools/libclang/CIndex.cpp
+++ tools/libclang/CIndex.cpp
@@ -6252,6 +6252,7 @@
case Decl::PragmaComment:
case Decl::PragmaDetectMismatch:
case Decl::UsingPack:
+ case Decl::Concept:
return C;
// Declaration kinds that don't make any sense here, but are
Index: test/Parser/cxx-concepts-requires-clause.cpp
===
--- /dev/null
+++ test/Parser/cxx-concepts-requires-clause.cpp
@@ -1,82 +0,0 @@
-// RUN: %clang_cc1 -std=c++14 -fconcepts-ts -x c++ %s -verify
-// expected-no-diagnostics
-
-// Test parsing of the optional requires-clause in a template-declaration.
-
-template requires true
-void foo() { }
-
-
-template requires !0
-struct A {
- void foo();
- struct AA;
- enum E : int;
- static int x;
-
- template requires true
- void Mfoo();
-
- template requires true
- struct M;
-
- template requires true
- static int Mx;
-
- template requires true
- using MQ = M;
-};
-
-template requires !0
-void A::foo() { }
-
-template requires !0
-struct A::AA { };
-
-template requires !0
-enum A::E : int { E0 };
-
-template requires !0
-int A::x = 0;
-
-template requires !0
-template requires true
-void A::Mfoo() { }
-
-template requires !0
-template requires true
-struct A::M { };
-
-template requires !0
-template requires true
-int A::Mx = 0;
-
-
-template requires true
-int x = 0;
-
-template requires true
-using Q = A;
-
-struct C {
- template requires true
- void Mfoo();
-
- template requires true
- struct M;
-
- template requires true
- static int Mx;
-
- template requires true
- using MQ = M;
-};
-
-template requires true
-void C::Mfoo() { }
-
-template requires true
-struct C::M { };
-
-template requires true
-int C::Mx = 0;
Index: test/Parser/cxx-concepts-ambig-constraint-expr.cpp
===
--- /dev/null
+++ test/Parser/cxx-concepts-ambig-constraint-expr.cpp
@@ -1,29 +0,0 @@
-// RUN: %clang_cc1 -std=c++14 -fconcepts-ts -x c++ %s -verify
-
-// Test parsing of constraint-expressions in cases where the grammar is
-// ambiguous with the expectation that the longest token sequence which matches
-// the syntax is consumed without backtracking.
-
-// type-specifier-seq in conversion-type-id
-template requires (bool)::operator short
-unsigned int foo(); // expected-error {{C++ requires a type specifier for all declarations}}
-
-// type-specifier-seq in new-type-id
-template requires (bool)sizeof new (T::f()) short
-unsigned int bar(); // expected-error {{C++ requires a type specifier for all declarations}}
-
-template requires (bool)sizeof new (T::f()) unsigned // expected-error {{'struct' cannot be signed or unsigned}}
-struct X { }; // expected-error {{'X' cannot be defined in a type specifier}}
-
-// C-style cast
-// of function call on function-style cast
-template requires (bool(T()))
-T (*fp)(); // expected-error {{use of undeclared identifier 'fp'}}
-
-// function-style cast
-// as the callee in a function call
-struct A {
- static int t;
- template requires bool(T())
- (A(T ())) { } // expected-error {{called object type 'bool' is not a function or function pointer}}
-};
Index: test/Parser/cxx2a-concept-declaration.cpp
[PATCH] D40381: Parse concept definition
rsmith added a comment.
Thanks!
Please revert the (presumably unintended) mode changes to the `ptxas`
executables.
Comment at: include/clang/AST/DeclTemplate.h:3035
+ SourceRange getSourceRange() const override LLVM_READONLY {
+return SourceRange(getLocation(), getLocation());
+ }
saar.raz wrote:
> rsmith wrote:
> > faisalv wrote:
> > > why not just fix it now?
> > > return SourceRange(getTemplateParameters()->getTemplateLoc(),
> > > ConstraintExpr->getLocEnd();
> > >
> > > ?
> > There's a bigger problem here:
> >
> > ```
> > TemplateDecl *TD = /*some ConceptDecl*/;
> > auto SR = TD->getSourceRange(); // crash
> > ```
> >
> > `TemplateDecl` has a hard assumption that it contains a `TemplatedDecl`.
> > So, three options:
> >
> > 1) Change `TemplateDecl` and all its users to remove this assumption (hard
> > and leads to an awkward-to-use AST)
> > 2) Add a `Decl` subclass that acts as the templated declaration in a
> > concept declaration (corresponding to the C++ grammar's
> > //concept-definition// production)
> > 3) Make `ConceptDecl` //not// derive from a `TemplateDecl` at all
> >
> > I think option 2 is my preference, but option 3 is also somewhat appealing
> > (there are other ways in which a concept is not a template: for instance,
> > it cannot be constrained, and it cannot be classified as either a type
> > template or a non-type template, because its kind depends on the context in
> > which it appears).
> >
> > Of course, this leads to one of the Hard Problems Of Computer Science:
> > naming things. `ConceptDecl` for a //concept-definition// and
> > `ConceptTemplateDecl` for the //template-head concept-definition// would be
> > consistent with the rest of the AST. It's a little unfortunate for the
> > longer name to be the AST node that we actually interact with, but the
> > consistency is probably worth the cost.
> The dummy decl is pretty confusing and will be a very weird decl in and of
> itself. I can't think of a good name right now, but your proposed naming will
> be pretty confusing given that a ConceptTemplateDecl does not template a
> concept declaration given the meaning of the phrase in the standard... Maybe
> ConceptBodyDecl? ConceptDummyDecl? ConceptDefinitionDecl? We need something
> that screams "this is not interesting" for the AST usage to be reasonable.
> Option 3 feels like loads of code duplication.
> I'm not entirely sure how many blind (through TemplateDecl) uses of
> getTemplatedDecl there are, but there might not be that many, in which case
> the first option might be the lesser of all these evils.
Faisal's comment is marked "Done" but not done.
`ConceptBodyDecl` or something like it seems reasonable. But I think we can
consider that after landing this patch, and leave the templated declaration
null for now.
Comment at: include/clang/AST/DeclTemplate.h:3016
+// \brief Declaration of a C++2a concept.
+class ConceptDecl : public TemplateDecl {
+protected:
This should also derive from `Mergeable`, since we are permitted
to merge multiple definitions of the same concept across translation units by
C++20 [basic.def.odr]/12.
Comment at: include/clang/Basic/DiagnosticParseKinds.td:1260-1270
+
+def err_concept_definition_unexpected_scope_spec : Error<
+ "unexpected namespace scope before concept name; concepts must be defined "
+ "inside their namespace, if any">;
+
+def err_concept_definition_not_identifier : Error<
+ "name defined in concept definition must be an identifier">;
Generally we don't leave blank lines between diagnostic definitions, and
instead use the continuation indent to visually separate them.
Comment at: include/clang/Basic/DiagnosticParseKinds.td:1261-1263
+def err_concept_definition_unexpected_scope_spec : Error<
+ "unexpected namespace scope before concept name; concepts must be defined "
+ "inside their namespace, if any">;
The phrasing of this (particularly the "if any") is a little confusing. I think
it's fine to just use the `err_concept_definition_not_identifier` diagnostic
for this case.
Comment at: include/clang/Basic/DiagnosticParseKinds.td:1269
+def err_concept_legacy_bool_keyword : Error<
+ "'bool' keyword after 'concept' is no longer valid in C++2a; remove it">;
+
I'd probably phrase this as
"ISO C++2a does not permit the 'bool' keyword after 'concept'"
(The Concepts TS isn't really the past -- TSes are more like an alternative
reality -- so "no longer" is a bit odd.) I'd also be tempted to turn this into
an `ExtWarn` so that we can accept code targeting the Concepts TS with a
warning.
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2443
+ "concept template parameter list must have at least one parameter; explicit "
+ "specialization of concepts is not allowed, did you attempt this?">;
[PATCH] D40381: Parse concept definition
saar.raz updated this revision to Diff 194417.
saar.raz added a comment.
Herald added a subscriber: jfb.
Herald added a project: clang.
Address most of rsmith's CR comments, rebase onto trunk
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D40381/new/
https://reviews.llvm.org/D40381
Files:
include/clang/AST/DeclTemplate.h
include/clang/AST/RecursiveASTVisitor.h
include/clang/Basic/DeclNodes.td
include/clang/Basic/DiagnosticParseKinds.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/TemplateKinds.h
include/clang/Parse/Parser.h
include/clang/Sema/Sema.h
include/clang/Serialization/ASTBitCodes.h
lib/AST/ASTDumper.cpp
lib/AST/DeclBase.cpp
lib/AST/DeclPrinter.cpp
lib/AST/DeclTemplate.cpp
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CodeGenModule.cpp
lib/Index/IndexDecl.cpp
lib/Parse/ParseTemplate.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaDeclCXX.cpp
lib/Sema/SemaLookup.cpp
lib/Sema/SemaTemplate.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
lib/Serialization/ASTCommon.cpp
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriter.cpp
lib/Serialization/ASTWriterDecl.cpp
test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp
test/Driver/Inputs/CUDA-nolibdevice/usr/local/cuda/bin/ptxas
test/Driver/Inputs/CUDA-symlinks/opt/cuda/bin/ptxas
test/Driver/Inputs/CUDA/usr/local/cuda/bin/ptxas
test/Parser/cxx-concept-declaration.cpp
test/Parser/cxx-concepts-ambig-constraint-expr.cpp
test/Parser/cxx-concepts-requires-clause.cpp
test/Parser/cxx2a-concept-declaration.cpp
test/Parser/cxx2a-concepts-ambig-constraint-expr.cpp
test/Parser/cxx2a-concepts-requires-clause.cpp
tools/libclang/CIndex.cpp
Index: tools/libclang/CIndex.cpp
===
--- tools/libclang/CIndex.cpp
+++ tools/libclang/CIndex.cpp
@@ -6252,6 +6252,7 @@
case Decl::PragmaComment:
case Decl::PragmaDetectMismatch:
case Decl::UsingPack:
+ case Decl::Concept:
return C;
// Declaration kinds that don't make any sense here, but are
Index: test/Parser/cxx-concepts-requires-clause.cpp
===
--- /dev/null
+++ test/Parser/cxx-concepts-requires-clause.cpp
@@ -1,82 +0,0 @@
-// RUN: %clang_cc1 -std=c++14 -fconcepts-ts -x c++ %s -verify
-// expected-no-diagnostics
-
-// Test parsing of the optional requires-clause in a template-declaration.
-
-template requires true
-void foo() { }
-
-
-template requires !0
-struct A {
- void foo();
- struct AA;
- enum E : int;
- static int x;
-
- template requires true
- void Mfoo();
-
- template requires true
- struct M;
-
- template requires true
- static int Mx;
-
- template requires true
- using MQ = M;
-};
-
-template requires !0
-void A::foo() { }
-
-template requires !0
-struct A::AA { };
-
-template requires !0
-enum A::E : int { E0 };
-
-template requires !0
-int A::x = 0;
-
-template requires !0
-template requires true
-void A::Mfoo() { }
-
-template requires !0
-template requires true
-struct A::M { };
-
-template requires !0
-template requires true
-int A::Mx = 0;
-
-
-template requires true
-int x = 0;
-
-template requires true
-using Q = A;
-
-struct C {
- template requires true
- void Mfoo();
-
- template requires true
- struct M;
-
- template requires true
- static int Mx;
-
- template requires true
- using MQ = M;
-};
-
-template requires true
-void C::Mfoo() { }
-
-template requires true
-struct C::M { };
-
-template requires true
-int C::Mx = 0;
Index: test/Parser/cxx-concepts-ambig-constraint-expr.cpp
===
--- /dev/null
+++ test/Parser/cxx-concepts-ambig-constraint-expr.cpp
@@ -1,29 +0,0 @@
-// RUN: %clang_cc1 -std=c++14 -fconcepts-ts -x c++ %s -verify
-
-// Test parsing of constraint-expressions in cases where the grammar is
-// ambiguous with the expectation that the longest token sequence which matches
-// the syntax is consumed without backtracking.
-
-// type-specifier-seq in conversion-type-id
-template requires (bool)::operator short
-unsigned int foo(); // expected-error {{C++ requires a type specifier for all declarations}}
-
-// type-specifier-seq in new-type-id
-template requires (bool)sizeof new (T::f()) short
-unsigned int bar(); // expected-error {{C++ requires a type specifier for all declarations}}
-
-template requires (bool)sizeof new (T::f()) unsigned // expected-error {{'struct' cannot be signed or unsigned}}
-struct X { }; // expected-error {{'X' cannot be defined in a type specifier}}
-
-// C-style cast
-// of function call on function-style cast
-template requires (bool(T()))
-T (*fp)(); // expected-error {{use of undeclared identifier 'fp'}}
-
-// function-style cast
-// as the callee in a function call
-struct A {
- static int t;
- template requires bool(T())
- (A(T ())) { } // expected-error {{called object type 'bool' is not a
[PATCH] D40381: Parse concept definition
saar.raz marked 21 inline comments as done. saar.raz added a comment. Only the TemplatedDecl issue remains, all other comments have been addressed and we're rebased onto master. @rsmith please reply to the last comment, and lets finally start merging these :) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D40381/new/ https://reviews.llvm.org/D40381 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
saar.raz added inline comments.
Herald added a subscriber: arphaman.
Comment at: include/clang/AST/DeclTemplate.h:3035
+ SourceRange getSourceRange() const override LLVM_READONLY {
+return SourceRange(getLocation(), getLocation());
+ }
rsmith wrote:
> faisalv wrote:
> > why not just fix it now?
> > return SourceRange(getTemplateParameters()->getTemplateLoc(),
> > ConstraintExpr->getLocEnd();
> >
> > ?
> There's a bigger problem here:
>
> ```
> TemplateDecl *TD = /*some ConceptDecl*/;
> auto SR = TD->getSourceRange(); // crash
> ```
>
> `TemplateDecl` has a hard assumption that it contains a `TemplatedDecl`. So,
> three options:
>
> 1) Change `TemplateDecl` and all its users to remove this assumption (hard
> and leads to an awkward-to-use AST)
> 2) Add a `Decl` subclass that acts as the templated declaration in a concept
> declaration (corresponding to the C++ grammar's //concept-definition//
> production)
> 3) Make `ConceptDecl` //not// derive from a `TemplateDecl` at all
>
> I think option 2 is my preference, but option 3 is also somewhat appealing
> (there are other ways in which a concept is not a template: for instance, it
> cannot be constrained, and it cannot be classified as either a type template
> or a non-type template, because its kind depends on the context in which it
> appears).
>
> Of course, this leads to one of the Hard Problems Of Computer Science: naming
> things. `ConceptDecl` for a //concept-definition// and `ConceptTemplateDecl`
> for the //template-head concept-definition// would be consistent with the
> rest of the AST. It's a little unfortunate for the longer name to be the AST
> node that we actually interact with, but the consistency is probably worth
> the cost.
The dummy decl is pretty confusing and will be a very weird decl in and of
itself. I can't think of a good name right now, but your proposed naming will
be pretty confusing given that a ConceptTemplateDecl does not template a
concept declaration given the meaning of the phrase in the standard... Maybe
ConceptBodyDecl? ConceptDummyDecl? ConceptDefinitionDecl? We need something
that screams "this is not interesting" for the AST usage to be reasonable.
Option 3 feels like loads of code duplication.
I'm not entirely sure how many blind (through TemplateDecl) uses of
getTemplatedDecl there are, but there might not be that many, in which case the
first option might be the lesser of all these evils.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D40381/new/
https://reviews.llvm.org/D40381
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
rsmith added inline comments.
Comment at: include/clang/AST/DeclTemplate.h:3007
+/// \brief Definition of concept, not just declaration actually.
+class ConceptDecl : public TemplateDecl {
This comment isn't appropriate. Please just describe what the node is. (And
note that a definition is a kind of declaration, despite common parlance.)
Comment at: include/clang/AST/DeclTemplate.h:3035
+ SourceRange getSourceRange() const override LLVM_READONLY {
+return SourceRange(getLocation(), getLocation());
+ }
faisalv wrote:
> why not just fix it now?
> return SourceRange(getTemplateParameters()->getTemplateLoc(),
> ConstraintExpr->getLocEnd();
>
> ?
There's a bigger problem here:
```
TemplateDecl *TD = /*some ConceptDecl*/;
auto SR = TD->getSourceRange(); // crash
```
`TemplateDecl` has a hard assumption that it contains a `TemplatedDecl`. So,
three options:
1) Change `TemplateDecl` and all its users to remove this assumption (hard and
leads to an awkward-to-use AST)
2) Add a `Decl` subclass that acts as the templated declaration in a concept
declaration (corresponding to the C++ grammar's //concept-definition//
production)
3) Make `ConceptDecl` //not// derive from a `TemplateDecl` at all
I think option 2 is my preference, but option 3 is also somewhat appealing
(there are other ways in which a concept is not a template: for instance, it
cannot be constrained, and it cannot be classified as either a type template or
a non-type template, because its kind depends on the context in which it
appears).
Of course, this leads to one of the Hard Problems Of Computer Science: naming
things. `ConceptDecl` for a //concept-definition// and `ConceptTemplateDecl`
for the //template-head concept-definition// would be consistent with the rest
of the AST. It's a little unfortunate for the longer name to be the AST node
that we actually interact with, but the consistency is probably worth the cost.
Comment at: include/clang/AST/DeclTemplate.h:3027
+ }
+
+ // TODO: Should do source range properly.
saar.raz wrote:
> Add setConstraintExpr (for use when calling CreateDeserialized, see
> ASTDeclReader)
Please remove this again. `ASTDeclReader` should set the `ConstraintExpr` field
directly. The AST is intended to be immutable after creation, so should
generally not have setters.
Comment at: include/clang/AST/RecursiveASTVisitor.h:1728
+ TRY_TO(TraverseStmt(D->getConstraintExpr()));
+ // FIXME: Traverse all the concept specializations (one we implement forming
template-ids with them).
+})
Hmm, concepts don't really have specializations, though, do they? (Much like
alias templates.) And because they're substituted incrementally, and the result
of evaluation can vary at different points in the same translation unit, it's
not obvious how much we can actually cache.
I suppose I'll see this was handled in later patches in the series :)
Comment at: include/clang/Basic/DiagnosticParseKinds.td:1134
+def err_concept_unexpected_scope_spec : Error<
+ "concepts must be defined within their namespace">;
+}
I would expect something more general here. For an alias-declaration, we say:
"error: name defined in alias declaration must be an identifier"
This is then also appropriate for other kinds of invalid //concept-name//s such
as
```
template concept operator int = true;
```
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2390-2391
"%select{explicitly instantiated|explicitly specialized|partially
specialized}1">;
+def err_concept_initialized_with_non_bool_type : Error<
+ "constraint expression must be 'bool' but found %0">;
+def err_concept_decls_may_only_appear_in_global_namespace_scope : Error<
"must be 'bool'" doesn't make sense. Maybe "constraint expression must be of
type 'bool' but is of type %0" or similar?
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2394-2395
+ "concept declarations may only appear in global or namespace scope">;
+def err_concept_no_associated_constraints : Error<
+ "concept may not have associated constraints">;
+def err_concept_not_implemented : Error<
Do not use "may not" in this context; it's ambiguous (this could be read as
"I'm not sure if this concept has associated constraints"). Use "cannot"
instead.
(And generally prefer "can" over "may" or "must" in diagnostics.)
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2396-2397
+ "concept may not have associated constraints">;
+def err_concept_not_implemented : Error<
+ "this part's not implemented yet">;
+def err_concept_no_explicit_specialization : Error<
Please make this a bit less informal and a little more informative. Perhaps
"sorry, unimplemented concepts feature used". For a temporary "under
[PATCH] D40381: Parse concept definition
faisalv added a comment. I discussed this briefly w Hubert - and i'm planning on modifying this patch slightly so that it flows through ParseDeclSpecifier and handles attributes and other invalid decl-specifiers such as static etc. more gracefully on a concept decl. I have this partially implemented - my hope is to get this done v soonish so feel free to ping me if you don't hear anything about this in a week or so ... https://reviews.llvm.org/D40381 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
saar.raz added inline comments.
Comment at: lib/Parse/ParseTemplate.cpp:383
+
+ if (!Tok.is(tok::identifier)) {
+Diag(Tok.getLocation(), diag::err_expected) << tok::identifier;
We could accept 'bool' here to be nice to people coming in from the old
Concepts TS version of these decls - and emit a proper warning.
https://reviews.llvm.org/D40381
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
saar.raz added inline comments. Comment at: lib/Parse/ParseTemplate.cpp:161 // Parse the actual template declaration. - return ParseSingleDeclarationAfterTemplate(Context, - ParsedTemplateInfo(, - isSpecialization, - LastParamListWasEmpty), - ParsingTemplateParams, - DeclEnd, AS, AccessAttrs); + if (!TryConsumeToken(tok::kw_concept)) +return ParseSingleDeclarationAfterTemplate(Context, Perhaps add a LangOpts.ConceptsTS check here? https://reviews.llvm.org/D40381 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
changyu added a comment.
I moved some template param checks from
`Parser::ParseTemplateDeclarationOrSpecialization` to
`Parser::ParseConceptDefinition` and `Sema::ActOnConceptDefinition`.
Comment at: lib/Parse/ParseTemplate.cpp:181
+TemplateParameterList *TPL = ParamLists[0];
+if (TPL->getLAngleLoc().getLocWithOffset(1) == TPL->getRAngleLoc()) {
+ Diag(TPL->getTemplateLoc(),
faisalv wrote:
> changyu wrote:
> > There's one problem here.
> >
> > I added this `if` in attempt to catch the following case (but it's wrong)
> > ```
> > template<> concept D1 = true; // expected-error {{expected template
> > parameter}}
> > ```
> > The problem is I'm not sure how to differentiate between the above
> > situation and the following
> > ```
> > template concept D1 = true; // expected-error {{expected
> > template parameter}}
> > ```
> > Both have an empty template parameter list. The latter case has diagnostic
> > printed by `ParseNonTypeTemplateParameter` while the former has not (so we
> > try to catch it here).
> >
> > What should we do?
> >
>
> I was thinking that we would just emit a (redundant in the case of a bad
> template parameter) message in Sema if the template-parameters are empty that
> explicit specializations are not allowed here. while it would be a little
> misleading in the invalid template parameter case - to fix this robustly
> would require some fine-tuning and correcting some of the
> handshaking/error-propagation between the parsing of the template parameters
> and the code that calls it, I think. I would vote for not holding up this
> patch for that, unless you feel strongly you'd like to fix that behavior -
> then we can try and work on that first?
>
> Thoughts?
>
>
Sure, let's fix that in another patch. I added a note for this in
cxx2a-concept-declaration.cpp.
https://reviews.llvm.org/D40381
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
changyu updated this revision to Diff 128123.
changyu marked 2 inline comments as done.
https://reviews.llvm.org/D40381
Files:
include/clang/AST/DeclTemplate.h
include/clang/AST/RecursiveASTVisitor.h
include/clang/Basic/DeclNodes.td
include/clang/Basic/DiagnosticParseKinds.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/TemplateKinds.h
include/clang/Parse/Parser.h
include/clang/Sema/Sema.h
include/clang/Serialization/ASTBitCodes.h
lib/AST/ASTDumper.cpp
lib/AST/DeclBase.cpp
lib/AST/DeclTemplate.cpp
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CodeGenModule.cpp
lib/Parse/ParseTemplate.cpp
lib/Sema/SemaTemplate.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
lib/Serialization/ASTCommon.cpp
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriter.cpp
lib/Serialization/ASTWriterDecl.cpp
test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp
test/Parser/cxx-concept-declaration.cpp
test/Parser/cxx2a-concept-declaration.cpp
tools/libclang/CIndex.cpp
Index: tools/libclang/CIndex.cpp
===
--- tools/libclang/CIndex.cpp
+++ tools/libclang/CIndex.cpp
@@ -5906,6 +5906,7 @@
case Decl::PragmaComment:
case Decl::PragmaDetectMismatch:
case Decl::UsingPack:
+ case Decl::Concept:
return C;
// Declaration kinds that don't make any sense here, but are
Index: test/Parser/cxx2a-concept-declaration.cpp
===
--- /dev/null
+++ test/Parser/cxx2a-concept-declaration.cpp
@@ -0,0 +1,43 @@
+
+// Support parsing of concepts
+
+// RUN: %clang_cc1 -std=c++2a -fconcepts-ts -verify %s
+template concept C1 = true;
+
+template concept C1 = true; // expected-error {{redefinition of 'C1'}}
+
+// The following case shouldn't give error about explicit specialization.
+// But the way we parse template parameters in ParseTemplateParameters right
+// now doesn't allow us to distinguish between a template parameter list with
+// an invalid template parameter and a template parameter list with with no
+// parameters.
+// TODO: Fix that.
+template concept D1 = true; // expected-error {{expected template parameter}} expected-error {{concept maybe not be explicitly specialized}}
+
+template<> concept D1 = true; // expected-error {{concept maybe not be explicitly specialized}}
+
+// TODO: Saar's patch should have this working.
+// template concept C2 = 0.f; // expected error {{constraint expression must be 'bool'}}
+
+struct S1 {
+ template concept C1 = true; // expected-error {{concept declarations may only appear in global or namespace scope}}
+};
+
+template
+template
+concept C4 = true; // expected-error {{extraneous template parameter list in concept definition}}
+
+template concept C5 = true; // expected-note {{previous}} expected-note {{previous}}
+int C5; // expected-error {{redefinition}}
+struct C5 {}; // expected-error {{redefinition}}
+
+struct C6 {};
+template concept C6 = true; // expected-error {{redefinition of 'C6' as different kind of symbol}}
+
+namespace thing {};
+
+template concept thing::C7 = true; // expected-error {{concepts must be defined within their namespace}}
+
+
+// TODO: Add test to prevent explicit specialization, partial specialization
+// and explicit instantiation of concepts.
Index: test/Parser/cxx-concept-declaration.cpp
===
--- test/Parser/cxx-concept-declaration.cpp
+++ /dev/null
@@ -1,7 +0,0 @@
-
-// Support parsing of concepts
-// Disabled for now.
-// expected-no-diagnostics
-
-// RUN: %clang_cc1 -std=c++14 -fconcepts-ts -x c++ -verify %s
-// template concept C1 = true;
Index: test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp
===
--- /dev/null
+++ test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp
@@ -0,0 +1,6 @@
+// RUN: %clang_cc1 -std=c++2a -fconcepts-ts -verify %s
+// expected-no-diagnostics
+
+// TODO: Make this work.
+// template concept C = true;
+// static_assert(C);
Index: lib/Serialization/ASTWriterDecl.cpp
===
--- lib/Serialization/ASTWriterDecl.cpp
+++ lib/Serialization/ASTWriterDecl.cpp
@@ -101,6 +101,7 @@
void VisitBindingDecl(BindingDecl *D);
void VisitNonTypeTemplateParmDecl(NonTypeTemplateParmDecl *D);
void VisitTemplateDecl(TemplateDecl *D);
+void VisitConceptDecl(ConceptDecl *D);
void VisitRedeclarableTemplateDecl(RedeclarableTemplateDecl *D);
void VisitClassTemplateDecl(ClassTemplateDecl *D);
void VisitVarTemplateDecl(VarTemplateDecl *D);
@@ -1385,6 +1386,12 @@
Record.AddTemplateParameterList(D->getTemplateParameters());
}
+void ASTDeclWriter::VisitConceptDecl(ConceptDecl *D) {
+ VisitTemplateDecl(D);
+ Record.AddStmt(D->getConstraintExpr());
+ Code = serialization::DECL_CONCEPT;
+}
+
void
[PATCH] D40381: Parse concept definition
faisalv added inline comments.
Comment at: lib/Parse/ParseTemplate.cpp:181
+TemplateParameterList *TPL = ParamLists[0];
+if (TPL->getLAngleLoc().getLocWithOffset(1) == TPL->getRAngleLoc()) {
+ Diag(TPL->getTemplateLoc(),
changyu wrote:
> There's one problem here.
>
> I added this `if` in attempt to catch the following case (but it's wrong)
> ```
> template<> concept D1 = true; // expected-error {{expected template
> parameter}}
> ```
> The problem is I'm not sure how to differentiate between the above situation
> and the following
> ```
> template concept D1 = true; // expected-error {{expected
> template parameter}}
> ```
> Both have an empty template parameter list. The latter case has diagnostic
> printed by `ParseNonTypeTemplateParameter` while the former has not (so we
> try to catch it here).
>
> What should we do?
>
I was thinking that we would just emit a (redundant in the case of a bad
template parameter) message in Sema if the template-parameters are empty that
explicit specializations are not allowed here. while it would be a little
misleading in the invalid template parameter case - to fix this robustly would
require some fine-tuning and correcting some of the
handshaking/error-propagation between the parsing of the template parameters
and the code that calls it, I think. I would vote for not holding up this
patch for that, unless you feel strongly you'd like to fix that behavior - then
we can try and work on that first?
Thoughts?
https://reviews.llvm.org/D40381
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
changyu added a comment.
I don't have commit privilege. And also there's one more problem we might want
to address first.
Thank you.
Comment at: lib/Parse/ParseTemplate.cpp:181
+TemplateParameterList *TPL = ParamLists[0];
+if (TPL->getLAngleLoc().getLocWithOffset(1) == TPL->getRAngleLoc()) {
+ Diag(TPL->getTemplateLoc(),
There's one problem here.
I added this `if` in attempt to catch the following case (but it's wrong)
```
template<> concept D1 = true; // expected-error {{expected template
parameter}}
```
The problem is I'm not sure how to differentiate between the above situation
and the following
```
template concept D1 = true; // expected-error {{expected
template parameter}}
```
Both have an empty template parameter list. The latter case has diagnostic
printed by `ParseNonTypeTemplateParameter` while the former has not (so we try
to catch it here).
What should we do?
https://reviews.llvm.org/D40381
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
faisalv added a comment. I think this looks good enough to commit - do you have commit privileges - or do you need one of us to commit it for you? thank you! https://reviews.llvm.org/D40381 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
changyu updated this revision to Diff 128094.
changyu marked an inline comment as done.
https://reviews.llvm.org/D40381
Files:
include/clang/AST/DeclTemplate.h
include/clang/AST/RecursiveASTVisitor.h
include/clang/Basic/DeclNodes.td
include/clang/Basic/DiagnosticParseKinds.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/TemplateKinds.h
include/clang/Parse/Parser.h
include/clang/Sema/Sema.h
include/clang/Serialization/ASTBitCodes.h
lib/AST/ASTDumper.cpp
lib/AST/DeclBase.cpp
lib/AST/DeclTemplate.cpp
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CodeGenModule.cpp
lib/Parse/ParseTemplate.cpp
lib/Sema/SemaTemplate.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
lib/Serialization/ASTCommon.cpp
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriter.cpp
lib/Serialization/ASTWriterDecl.cpp
test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp
test/Parser/cxx-concept-declaration.cpp
test/Parser/cxx2a-concept-declaration.cpp
tools/libclang/CIndex.cpp
Index: tools/libclang/CIndex.cpp
===
--- tools/libclang/CIndex.cpp
+++ tools/libclang/CIndex.cpp
@@ -5906,6 +5906,7 @@
case Decl::PragmaComment:
case Decl::PragmaDetectMismatch:
case Decl::UsingPack:
+ case Decl::Concept:
return C;
// Declaration kinds that don't make any sense here, but are
Index: test/Parser/cxx2a-concept-declaration.cpp
===
--- /dev/null
+++ test/Parser/cxx2a-concept-declaration.cpp
@@ -0,0 +1,37 @@
+
+// Support parsing of concepts
+
+// RUN: %clang_cc1 -std=c++2a -fconcepts-ts -verify %s
+template concept C1 = true;
+
+template concept C1 = true; // expected-error {{redefinition of 'C1'}}
+
+template concept D1 = true; // expected-error {{expected template parameter}}
+
+template<> concept D1 = true; // expected-error {{expected template parameter}}
+
+// TODO:
+// template concept C2 = 0.f; // expected error {{constraint expression must be 'bool'}}
+
+struct S1 {
+ template concept C1 = true; // expected-error {{concept declarations may only appear in global or namespace scope}}
+};
+
+template
+template
+concept C4 = true; // expected-error {{extraneous template parameter list in concept definition}}
+
+template concept C5 = true; // expected-note {{previous}} expected-note {{previous}}
+int C5; // expected-error {{redefinition}}
+struct C5 {}; // expected-error {{redefinition}}
+
+struct C6 {};
+template concept C6 = true; // expected-error {{redefinition of 'C6' as different kind of symbol}}
+
+namespace thing {};
+
+template concept thing::C7 = true; // expected-error {{concepts must be defined within their namespace}}
+
+
+// TODO: Add test to prevent explicit specialization, partial specialization
+// and explicit instantiation of concepts.
Index: test/Parser/cxx-concept-declaration.cpp
===
--- test/Parser/cxx-concept-declaration.cpp
+++ /dev/null
@@ -1,7 +0,0 @@
-
-// Support parsing of concepts
-// Disabled for now.
-// expected-no-diagnostics
-
-// RUN: %clang_cc1 -std=c++14 -fconcepts-ts -x c++ -verify %s
-// template concept C1 = true;
Index: test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp
===
--- /dev/null
+++ test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp
@@ -0,0 +1,6 @@
+// RUN: %clang_cc1 -std=c++2a -fconcepts-ts -verify %s
+// expected-no-diagnostics
+
+// TODO: Make this work.
+// template concept C = true;
+// static_assert(C);
Index: lib/Serialization/ASTWriterDecl.cpp
===
--- lib/Serialization/ASTWriterDecl.cpp
+++ lib/Serialization/ASTWriterDecl.cpp
@@ -101,6 +101,7 @@
void VisitBindingDecl(BindingDecl *D);
void VisitNonTypeTemplateParmDecl(NonTypeTemplateParmDecl *D);
void VisitTemplateDecl(TemplateDecl *D);
+void VisitConceptDecl(ConceptDecl *D);
void VisitRedeclarableTemplateDecl(RedeclarableTemplateDecl *D);
void VisitClassTemplateDecl(ClassTemplateDecl *D);
void VisitVarTemplateDecl(VarTemplateDecl *D);
@@ -1385,6 +1386,12 @@
Record.AddTemplateParameterList(D->getTemplateParameters());
}
+void ASTDeclWriter::VisitConceptDecl(ConceptDecl *D) {
+ VisitTemplateDecl(D);
+ Record.AddStmt(D->getConstraintExpr());
+ Code = serialization::DECL_CONCEPT;
+}
+
void ASTDeclWriter::VisitRedeclarableTemplateDecl(RedeclarableTemplateDecl *D) {
VisitRedeclarable(D);
Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -1277,6 +1277,7 @@
RECORD(DECL_TEMPLATE_TYPE_PARM);
RECORD(DECL_NON_TYPE_TEMPLATE_PARM);
RECORD(DECL_TEMPLATE_TEMPLATE_PARM);
+ RECORD(DECL_CONCEPT);
RECORD(DECL_TYPE_ALIAS_TEMPLATE);
[PATCH] D40381: Parse concept definition
changyu marked 27 inline comments as done.
changyu added inline comments.
Comment at: lib/Sema/SemaTemplate.cpp:7713
+
+ if (!ConstraintExpr->isTypeDependent() &&
+ ConstraintExpr->getType() != Context.BoolTy) {
faisalv wrote:
> saar.raz wrote:
> > faisalv wrote:
> > > Consider refactoring these checks on constraint expressions into a
> > > separate function checkConstraintExpression (that we can also call from
> > > other contexts such as requires-clauses and nested requires expressions)?
> > I did that in the upcoming patches, no need to do it here as well.
> Once again - relying on a future patch (especially without a clear FIXME) to
> tweak the architectural/factorization issues that you have been made aware of
> (and especially those, such as this, that do not require too much effort), is
> not practice I would generally encourage.
>
> So changyu, if you agree that these suggestions would improve the quality of
> the patch and leave clang in a more robust state (maintenance-wise or
> execution-wise), then please make the changes. If not, please share your
> thoughts as to why these suggestions would either not be an improvement or be
> inconsistent with the theme of this patch.
>
> Thanks!
I removed it here since Saar fixed it in his patch.
https://reviews.llvm.org/D40381
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
faisalv added inline comments.
Comment at: lib/Sema/SemaTemplate.cpp:7735
+ ActOnDocumentableDecl(NewDecl);
+ CurContext->addDecl(NewDecl);
+ return NewDecl;
changyu wrote:
> faisalv wrote:
> > Why not use 'PushOnScopeChains' onto the enclosing scope?
> > Something along these lines:
> >assert(S->isTemplateParamScope() && S->getParent() &&
> > !S->getParent()->isTemplateParamScope() && "...");
> >PushOnScopeChains(NewDecl, S->getParent(),/*AddToContext*/true);
> The condition
> ```
> S->isTemplateParamScope()
> ```
> fails in this case
> ```
> template concept D1 = true; // expected-error {{expected template
> parameter}}
> ```
>
> `ParseTemplateDeclarationOrSpecialization` calls `ParseTemplateParameters`
> which eventually calls `ParseNonTypeTemplateParameter`.
> `ParseNonTypeTemplateParameter` prints the diag and fails but does not cause
> `ParseTemplateParameters` to fail (I'm not sure why).
> `ParseTemplateDeclarationOrSpecialization` proceeds to parse the concept
> definition, and we get this
>
> ```
> /home/changyu/test.cpp:1:10: error: expected template parameter
> template concept D1 = true; // expected-error {{expected template
> parameter}}
> ^
> clang: /home/changyu/git/llvm/tools/clang/lib/Sema/SemaTemplate.cpp:7747:
> clang::Decl* clang::Sema::ActOnConceptDefinition(clang::Scope*,
> clang::MultiTemplateParamsArg, clang::IdentifierInfo*, clang::SourceLocation,
> clang::Expr*): Assertion `S->isTemplateParamScope() && "Not in template param
> scope?"' failed.
> ```
>
> What should we do?
I think this occurs because our template parameter list is incorrectly
perceived as empty - i.e. once the error occurs - to continue processing errors
clang assumes this case is an explicit-specialization and replaces the template
parameter scope with the outer scope. I think the thing to do here - which
might also address the case where folks actually write 'template<> concept' is
to actually check if template-parameter-list is empty - and emit a diagnostic
about concepts can not be explicit specializations or some such ...
On a somewhat related note - i think the logic for signaling, handling and
propagating failures of parsing the template parameter list might be a little
broken (fixing which, might avoid triggering the assertion in template
but not template<>). I might submit a patch to fix that error handling-issue
separately - but either way I think we should handle the
explicit-specialization error?
Thoughts?
https://reviews.llvm.org/D40381
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
changyu marked 6 inline comments as done.
changyu added inline comments.
Comment at: lib/Sema/SemaTemplate.cpp:7735
+ ActOnDocumentableDecl(NewDecl);
+ CurContext->addDecl(NewDecl);
+ return NewDecl;
faisalv wrote:
> Why not use 'PushOnScopeChains' onto the enclosing scope?
> Something along these lines:
>assert(S->isTemplateParamScope() && S->getParent() &&
> !S->getParent()->isTemplateParamScope() && "...");
>PushOnScopeChains(NewDecl, S->getParent(),/*AddToContext*/true);
The condition
```
S->isTemplateParamScope()
```
fails in this case
```
template concept D1 = true; // expected-error {{expected template
parameter}}
```
`ParseTemplateDeclarationOrSpecialization` calls `ParseTemplateParameters`
which eventually calls `ParseNonTypeTemplateParameter`.
`ParseNonTypeTemplateParameter` prints the diag and fails but does not cause
`ParseTemplateParameters` to fail (I'm not sure why).
`ParseTemplateDeclarationOrSpecialization` proceeds to parse the concept
definition, and we get this
```
/home/changyu/test.cpp:1:10: error: expected template parameter
template concept D1 = true; // expected-error {{expected template
parameter}}
^
clang: /home/changyu/git/llvm/tools/clang/lib/Sema/SemaTemplate.cpp:7747:
clang::Decl* clang::Sema::ActOnConceptDefinition(clang::Scope*,
clang::MultiTemplateParamsArg, clang::IdentifierInfo*, clang::SourceLocation,
clang::Expr*): Assertion `S->isTemplateParamScope() && "Not in template param
scope?"' failed.
```
What should we do?
https://reviews.llvm.org/D40381
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
faisalv requested changes to this revision.
faisalv added inline comments.
This revision now requires changes to proceed.
Comment at: lib/Sema/SemaTemplate.cpp:3899
+ // constraint expressions right now.
+ return Template->getConstraintExpr();
+}
saar.raz wrote:
> faisalv wrote:
> > I don't think we want to just return the constraint expr? I think we would
> > need to create another conceptspecializationDecl node and nest it within a
> > DeclRefExpr? But once again, I think we should defer this to another patch
> > - no?
> This was meant as a placeholder. The next patch (D41217) replaces this
> function, I don't think it is that much of a big deal what happens here in
> this patch.
This: " I don't think it is that much of a big deal what happens here in this
patch." I think is poor practice in an open source project when you're not sure
when the next patch will land.
I think, when features are implemented incrementally - each patch should
diagnose the yet to be implemented features - and error out as gracefully as
possible. So for instance replacing the body of this function with an
appropriate diagnostic (that is then removed in future patches) would be the
better thing to do here.
Comment at: lib/Sema/SemaTemplate.cpp:7713
+
+ if (!ConstraintExpr->isTypeDependent() &&
+ ConstraintExpr->getType() != Context.BoolTy) {
saar.raz wrote:
> faisalv wrote:
> > Consider refactoring these checks on constraint expressions into a separate
> > function checkConstraintExpression (that we can also call from other
> > contexts such as requires-clauses and nested requires expressions)?
> I did that in the upcoming patches, no need to do it here as well.
Once again - relying on a future patch (especially without a clear FIXME) to
tweak the architectural/factorization issues that you have been made aware of
(and especially those, such as this, that do not require too much effort), is
not practice I would generally encourage.
So changyu, if you agree that these suggestions would improve the quality of
the patch and leave clang in a more robust state (maintenance-wise or
execution-wise), then please make the changes. If not, please share your
thoughts as to why these suggestions would either not be an improvement or be
inconsistent with the theme of this patch.
Thanks!
https://reviews.llvm.org/D40381
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
saar.raz added inline comments.
Comment at: lib/Sema/SemaTemplate.cpp:3899
+ // constraint expressions right now.
+ return Template->getConstraintExpr();
+}
faisalv wrote:
> I don't think we want to just return the constraint expr? I think we would
> need to create another conceptspecializationDecl node and nest it within a
> DeclRefExpr? But once again, I think we should defer this to another patch -
> no?
This was meant as a placeholder. The next patch (D41217) replaces this
function, I don't think it is that much of a big deal what happens here in this
patch.
Comment at: lib/Sema/SemaTemplate.cpp:7713
+
+ if (!ConstraintExpr->isTypeDependent() &&
+ ConstraintExpr->getType() != Context.BoolTy) {
faisalv wrote:
> Consider refactoring these checks on constraint expressions into a separate
> function checkConstraintExpression (that we can also call from other contexts
> such as requires-clauses and nested requires expressions)?
I did that in the upcoming patches, no need to do it here as well.
https://reviews.llvm.org/D40381
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
faisalv added a comment.
Thanks for working on this! :)
Comment at: include/clang/AST/DeclTemplate.h:3035
+ SourceRange getSourceRange() const override LLVM_READONLY {
+return SourceRange(getLocation(), getLocation());
+ }
why not just fix it now?
return SourceRange(getTemplateParameters()->getTemplateLoc(),
ConstraintExpr->getLocEnd();
?
Comment at: include/clang/AST/RecursiveASTVisitor.h:1725
+DEF_TRAVERSE_DECL(ConceptDecl, {})
+
Perhaps something along these lines?
DEF_TRAVERSE_DECL(ConceptDecl, {
TRY_TO(TraverseTemplateParameterListHelper(D->getTemplateParameters()));
TRY_TO(TraverseStmt(D->getConstraintExpr()));
// FIXME: Traverse all the concept specializations (one we implement
forming template-ids with them).
})
Comment at: include/clang/Sema/Sema.h:6196
+ const TemplateArgumentListInfo *TemplateArgs);
+
ExprResult BuildTemplateIdExpr(const CXXScopeSpec ,
Have you considered just deferring formation of concept template-ids to the
next patch (especially since it might require introduction of another AST node
ConceptSpecializationDecl. We could just focus on handling concept
declarations/definitions in this one (and emit an unimplemented error if
someone tries to form a template-id with a concept within BuildTemplateId etc.)
- but perhaps consider making sure we handle name clashes/redeclarations with
any other parsed names in the same namespace (within this patch)?
Comment at: lib/AST/DeclBase.cpp:773
+ return IDNS_Ordinary | IDNS_Tag;
+
// Never have names.
Pls consider just lumping this case with 'Binding/VarTemplate' (which by the
way also includes a comment for why we have to add the ugly IDNS_Tag hack here
(personally i think we should refactor this functionality, currently it seems
split between this and SemaLookup.cpp:getIDNS and would benefit from some sort
of well-designed cohesion - but that's another (low-priority) patch)
Comment at: lib/CodeGen/CGDecl.cpp:108
case Decl::Empty:
+ case Decl::Concept:
// None of these decls require codegen support.
You would need to add this to EmitTopLevelDecl too to handle global-namespace
concept declarations.
Comment at: lib/Parse/ParseTemplate.cpp:368
+
+ if (!TryConsumeToken(tok::equal)) {
+Diag(Tok.getLocation(), diag::err_expected) << "equal";
I think we should diagnose qualified-name errors here much more gracefully than
we do.
i.e. template concept N::C = ...
- perhaps try and parse a scope-specifier, and if found emit a diagnostic such
as concepts must be defined within their namespace ...
Comment at: lib/Sema/SemaTemplate.cpp:3899
+ // constraint expressions right now.
+ return Template->getConstraintExpr();
+}
I don't think we want to just return the constraint expr? I think we would need
to create another conceptspecializationDecl node and nest it within a
DeclRefExpr? But once again, I think we should defer this to another patch -
no?
Comment at: lib/Sema/SemaTemplate.cpp:3923
bool InstantiationDependent;
- if (R.getAsSingle() &&
- !TemplateSpecializationType::anyDependentTemplateArguments(
- *TemplateArgs, InstantiationDependent)) {
+ bool DependentArguments =
+TemplateSpecializationType::anyDependentTemplateArguments(
const bool, no?
Comment at: lib/Sema/SemaTemplate.cpp:7692
+ MultiTemplateParamsArg TemplateParameterLists,
+ IdentifierInfo *Name, SourceLocation L,
+ Expr *ConstraintExpr) {
Rename L as NameLoc?
Comment at: lib/Sema/SemaTemplate.cpp:7706
+ }
+
+ ConceptDecl *NewDecl = ConceptDecl::Create(Context, DC, L, Name,
Perhaps add a lookup check here? (or a fixme that we are deferring to the next
patch), something along these *pseudo-code* lines:
LookupResult Previous(*this,
DeclarationNameInfo(DeclarationName(Name),NameLoc), LookupOrdinaryName);
LookupName(Previous, S);
if (Previous.getRepresentativeDecl())... if the decl is a concept - give
error regarding only one definition allowed in a TU, if a different kind of
decl then declared w a different symbol type of error?
Comment at: lib/Sema/SemaTemplate.cpp:7713
+
+ if (!ConstraintExpr->isTypeDependent() &&
+ ConstraintExpr->getType() != Context.BoolTy) {
Consider refactoring these checks on constraint expressions into a separate
function checkConstraintExpression (that we can also call from other contexts
such as requires-clauses and nested requires expressions)?
Comment at: lib/Sema/SemaTemplate.cpp:7735
+
[PATCH] D40381: Parse concept definition
saar.raz requested changes to this revision. saar.raz added a comment. This revision now requires changes to proceed. Do the requested clang-formatting as shown by hubert, other than that looks good to me :) https://reviews.llvm.org/D40381 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
Rakete added inline comments. Comment at: lib/Sema/SemaTemplate.cpp:7693 +Decl *Sema::ActOnConceptDefinition(Scope *S, + MultiTemplateParamsArg TemplateParameterLists, + IdentifierInfo *Name, SourceLocation L, hubert.reinterpretcast wrote: > changyu wrote: > > Rakete wrote: > > > Did you run this through clang-format? > > No, when I run the file through clang-format (with no arguments except the > > file), it reformats the whole file. How should I be running clang-format? > One workflow that works is to clang-format the base file, clang-format with > your changes, grab a patch and then apply it to the original base file > (probably needs some manual work). I always copy the code I want to format, then pipe it into clang-format, and the paste it back with some manual adjustments to indentation. https://reviews.llvm.org/D40381 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
hubert.reinterpretcast added inline comments.
Comment at: include/clang/Sema/Sema.h:6194
+SourceLocation TemplateLoc,
+const TemplateArgumentListInfo *TemplateArgs);
+
changyu wrote:
> hubert.reinterpretcast wrote:
> > Indentation issue here too.
> That last line is 79 characters long.
clang-format is happy to give:
```
ExprResult
CheckConceptTemplateId(const CXXScopeSpec ,
const DeclarationNameInfo ,
ConceptDecl *Template, SourceLocation TemplateLoc,
const TemplateArgumentListInfo *TemplateArgs);
```
I'm no fan of blindly using clang-format, but its output is sometimes useful.
Comment at: lib/Parse/ParseTemplate.cpp:374
+
+ ExprResult ConstraintExpr = ParseConstraintExpression();
+
changyu wrote:
> hubert.reinterpretcast wrote:
> > changyu wrote:
> > > saar.raz wrote:
> > > > Add a check to ParseConstraintExpression that the type is either
> > > > dependent or bool, and add an apropriate diagnostic.
> > > >
> > > > ```
> > > > if (!ConstraintExpr->isTypeDependent() && ConstraintExpr->getType()
> > > > != Context.BoolTy) {
> > > > Diag(Init->getSourceRange().getBegin(),
> > > >diag::err_concept_initialized_with_non_bool_type) <<
> > > > Init->getType();
> > > > Concept->setInvalidDecl();
> > > > return;
> > > > }
> > > > ```
> > > I'm guessing you meant for this to be in `class Sema` so I added this to
> > > `Sema::ActOnConceptDefinition`. Also what is `Init` here?
> > I think that would still need a TODO to instead walk the constraint
> > expression for atomic constraints and diagnose those.
> > ```
> > template
> > concept C = 1 || T::value; // error
> > ```
> Why is that an error? [temp.constr.normal] in p0734r0 seems to say it's valid?
From N4700 subclause 17.4.1.2 [temp.constr.atomic] paragraph 3:
[ ... ], and E shall be a constant expression of type bool
A search of "bool" in P0734R0 seems to indicate that is also the basis for the
diagnostic Saar is requesting.
Although that wording only applies clearly when determining the satisfaction of
C for some T, it would be good to catch it early. I believe that the
particular case I presented falls under the "no valid specialization" wording
in [temp.res].
I think there is a gap between the wording and the intent if overloaded binary
logical operators, detectable without substitution, are not sufficiently wrong
on the part of the user that a compiler may refuse to translate the program.
Comment at: lib/Sema/SemaTemplate.cpp:7693
+Decl *Sema::ActOnConceptDefinition(Scope *S,
+ MultiTemplateParamsArg TemplateParameterLists,
+ IdentifierInfo *Name, SourceLocation L,
changyu wrote:
> Rakete wrote:
> > Did you run this through clang-format?
> No, when I run the file through clang-format (with no arguments except the
> file), it reformats the whole file. How should I be running clang-format?
One workflow that works is to clang-format the base file, clang-format with
your changes, grab a patch and then apply it to the original base file
(probably needs some manual work).
https://reviews.llvm.org/D40381
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
changyu added inline comments. Comment at: include/clang/Sema/Sema.h:6194 +SourceLocation TemplateLoc, +const TemplateArgumentListInfo *TemplateArgs); + hubert.reinterpretcast wrote: > Indentation issue here too. That last line is 79 characters long. https://reviews.llvm.org/D40381 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
changyu planned changes to this revision.
changyu marked 22 inline comments as done.
changyu added inline comments.
Comment at: lib/Parse/ParseTemplate.cpp:374
+
+ ExprResult ConstraintExpr = ParseConstraintExpression();
+
saar.raz wrote:
> Add a check to ParseConstraintExpression that the type is either dependent or
> bool, and add an apropriate diagnostic.
>
> ```
> if (!ConstraintExpr->isTypeDependent() && ConstraintExpr->getType() !=
> Context.BoolTy) {
> Diag(Init->getSourceRange().getBegin(),
>diag::err_concept_initialized_with_non_bool_type) <<
> Init->getType();
> Concept->setInvalidDecl();
> return;
> }
> ```
I'm guessing you meant for this to be in `class Sema` so I added this to
`Sema::ActOnConceptDefinition`. Also what is `Init` here?
Comment at: lib/Parse/ParseTemplate.cpp:374
+
+ ExprResult ConstraintExpr = ParseConstraintExpression();
+
hubert.reinterpretcast wrote:
> changyu wrote:
> > saar.raz wrote:
> > > Add a check to ParseConstraintExpression that the type is either
> > > dependent or bool, and add an apropriate diagnostic.
> > >
> > > ```
> > > if (!ConstraintExpr->isTypeDependent() && ConstraintExpr->getType()
> > > != Context.BoolTy) {
> > > Diag(Init->getSourceRange().getBegin(),
> > >diag::err_concept_initialized_with_non_bool_type) <<
> > > Init->getType();
> > > Concept->setInvalidDecl();
> > > return;
> > > }
> > > ```
> > I'm guessing you meant for this to be in `class Sema` so I added this to
> > `Sema::ActOnConceptDefinition`. Also what is `Init` here?
> I think that would still need a TODO to instead walk the constraint
> expression for atomic constraints and diagnose those.
> ```
> template
> concept C = 1 || T::value; // error
> ```
Why is that an error? [temp.constr.normal] in p0734r0 seems to say it's valid?
Comment at: lib/Sema/SemaTemplate.cpp:3903
+ // /*FoundD=*/nullptr, TemplateArgs);
+ return Template->getConstraintExpr();
+ return true;
hubert.reinterpretcast wrote:
> Add more comments here. It looks like this allows us to get id-expressions
> naming concepts defined with non-dependent `bool` constraint expressions to
> "work" for now?
That's pretty much it.
Comment at: lib/Sema/SemaTemplate.cpp:3936
+ if (R.getAsSingle()) {
+return CheckConceptTemplateId(SS, R.getLookupNameInfo(),
Rakete wrote:
> saar.raz wrote:
> > We're gonna want to check
> > ```
> > !TemplateSpecializationType::anyDependentTemplateArguments(*TemplateArgs,
> > InstantiationDependent)
> > ```
> > here as well - so that we can instantiate a ConceptSpecializationExpr later
> > when we have it (we're not gonna want to instantiate a
> > ConceptSpecializationExpr with dependent arguments.
> No braces here please.
I'm following the style of the surrounding code here. I can remove the braces
if you insist though.
Comment at: lib/Sema/SemaTemplate.cpp:7693
+Decl *Sema::ActOnConceptDefinition(Scope *S,
+ MultiTemplateParamsArg TemplateParameterLists,
+ IdentifierInfo *Name, SourceLocation L,
Rakete wrote:
> Did you run this through clang-format?
No, when I run the file through clang-format (with no arguments except the
file), it reformats the whole file. How should I be running clang-format?
https://reviews.llvm.org/D40381
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
hubert.reinterpretcast added inline comments.
Comment at: include/clang/Sema/Sema.h:6194
+SourceLocation TemplateLoc,
+const TemplateArgumentListInfo *TemplateArgs);
+
Indentation issue here too.
Comment at: lib/Parse/ParseTemplate.cpp:57
///
+/// template-declaration: [Concepts TS]
+/// template-head declaration
C++2a Concepts
Comment at: lib/Parse/ParseTemplate.cpp:374
+
+ ExprResult ConstraintExpr = ParseConstraintExpression();
+
saar.raz wrote:
> Add a check to ParseConstraintExpression that the type is either dependent or
> bool, and add an apropriate diagnostic.
>
> ```
> if (!ConstraintExpr->isTypeDependent() && ConstraintExpr->getType() !=
> Context.BoolTy) {
> Diag(Init->getSourceRange().getBegin(),
>diag::err_concept_initialized_with_non_bool_type) <<
> Init->getType();
> Concept->setInvalidDecl();
> return;
> }
> ```
I think that would still need a TODO to instead walk the constraint expression
for atomic constraints and diagnose those.
```
template
concept C = 1 || T::value; // error
```
Comment at: lib/Sema/SemaTemplate.cpp:3903
+ // /*FoundD=*/nullptr, TemplateArgs);
+ return Template->getConstraintExpr();
+ return true;
Add more comments here. It looks like this allows us to get id-expressions
naming concepts defined with non-dependent `bool` constraint expressions to
"work" for now?
Comment at: test/Parser/cxx-concept-declaration.cpp:5
// RUN: %clang_cc1 -std=c++14 -fconcepts-ts -x c++ -verify %s
-// template concept C1 = true;
+template concept C1 = true;
+
Should add tests to prevent redeclaring concept names as either a "normal"
(e.g., variable) or tag name.
Also the reverse for redeclaring a tag name as a concept.
Should add tests to prevent multiple definitions of the same concept.
Should eventually add tests to prevent explicit specialization, partial
specialization and explicit instantiation of concepts: while the restriction is
syntactic in the wording, that does not necessarily translate directly to the
implementation strategy. In any case, we may discover that we want a nicer
message.
Should add tests to prevent defining concepts at class scope.
https://reviews.llvm.org/D40381
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
saar.raz added inline comments. Comment at: test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp:1 +// RUN: %clang_cc1 -std=c++1z -fconcepts-ts -fcxx-exceptions -x c++ -verify %s +// expected-no-diagnostics hubert.reinterpretcast wrote: > saar.raz wrote: > > Rakete wrote: > > > There is no `-fconcepts-ts` flag AFAIK. Also, why `-fcxx-exceptions`, > > > it's not needed here. The `-x c++` part too. It should also be > > > `-std=c++2a`. > > There is a -fconcepts-ts flag (clang -Xclang -fconcepts-ts), I believe now > > that there is no more Concepts TS the flag should be removed as well and > > all concepts-related code be put under C++2a ifs. > This was discussed with Richard, Faisal, and Aaron Ballman before. We would > like to use -Xclang -fconcepts until there is enough of an implementation. > When we are ready, then we switch to having it directly under C++2a. There is > no reason why dealing with the option cannot be done as a separate patch, so > I don't think we need to hold up this one for that work. OK, wasn't aware this was discussed before. I agree then, leave the -fconcepts-ts for now. https://reviews.llvm.org/D40381 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
hubert.reinterpretcast added inline comments. Comment at: test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp:1 +// RUN: %clang_cc1 -std=c++1z -fconcepts-ts -fcxx-exceptions -x c++ -verify %s +// expected-no-diagnostics saar.raz wrote: > Rakete wrote: > > There is no `-fconcepts-ts` flag AFAIK. Also, why `-fcxx-exceptions`, it's > > not needed here. The `-x c++` part too. It should also be `-std=c++2a`. > There is a -fconcepts-ts flag (clang -Xclang -fconcepts-ts), I believe now > that there is no more Concepts TS the flag should be removed as well and all > concepts-related code be put under C++2a ifs. This was discussed with Richard, Faisal, and Aaron Ballman before. We would like to use -Xclang -fconcepts until there is enough of an implementation. When we are ready, then we switch to having it directly under C++2a. There is no reason why dealing with the option cannot be done as a separate patch, so I don't think we need to hold up this one for that work. https://reviews.llvm.org/D40381 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
saar.raz added inline comments. Comment at: test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp:1 +// RUN: %clang_cc1 -std=c++1z -fconcepts-ts -fcxx-exceptions -x c++ -verify %s +// expected-no-diagnostics Rakete wrote: > There is no `-fconcepts-ts` flag AFAIK. Also, why `-fcxx-exceptions`, it's > not needed here. The `-x c++` part too. It should also be `-std=c++2a`. There is a -fconcepts-ts flag (clang -Xclang -fconcepts-ts), I believe now that there is no more Concepts TS the flag should be removed as well and all concepts-related code be put under C++2a ifs. https://reviews.llvm.org/D40381 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
Rakete added inline comments.
Comment at: lib/Parse/ParseTemplate.cpp:161
// Parse the actual template declaration.
- return ParseSingleDeclarationAfterTemplate(Context,
- ParsedTemplateInfo(,
- isSpecialization,
-
LastParamListWasEmpty),
- ParsingTemplateParams,
- DeclEnd, AS, AccessAttrs);
+ if (!TryConsumeToken(tok::kw_concept)) {
+return ParseSingleDeclarationAfterTemplate(Context,
No braces here please.
Comment at: lib/Parse/ParseTemplate.cpp:168
+ DeclEnd, AS, AccessAttrs);
+ } else {
+return ParseConceptDefinition(Context,
No `else` needed here.
Comment at: lib/Parse/ParseTemplate.cpp:382
+
+ return ThisDecl;
+}
Do we really need `ThisDecl`?
Comment at: lib/Sema/SemaTemplate.cpp:3936
+ if (R.getAsSingle()) {
+return CheckConceptTemplateId(SS, R.getLookupNameInfo(),
saar.raz wrote:
> We're gonna want to check
> ```
> !TemplateSpecializationType::anyDependentTemplateArguments(*TemplateArgs,
> InstantiationDependent)
> ```
> here as well - so that we can instantiate a ConceptSpecializationExpr later
> when we have it (we're not gonna want to instantiate a
> ConceptSpecializationExpr with dependent arguments.
No braces here please.
Comment at: lib/Sema/SemaTemplate.cpp:3938
+return CheckConceptTemplateId(SS, R.getLookupNameInfo(),
+ R.getAsSingle(),
+ TemplateKWLoc, TemplateArgs);
clang-format please :)
Comment at: lib/Sema/SemaTemplate.cpp:7693
+Decl *Sema::ActOnConceptDefinition(Scope *S,
+ MultiTemplateParamsArg TemplateParameterLists,
+ IdentifierInfo *Name, SourceLocation L,
Did you run this through clang-format?
Comment at: test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp:1
+// RUN: %clang_cc1 -std=c++1z -fconcepts-ts -fcxx-exceptions -x c++ -verify %s
+// expected-no-diagnostics
There is no `-fconcepts-ts` flag AFAIK. Also, why `-fcxx-exceptions`, it's not
needed here. The `-x c++` part too. It should also be `-std=c++2a`.
https://reviews.llvm.org/D40381
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
saar.raz requested changes to this revision.
saar.raz added a comment.
This revision now requires changes to proceed.
Also add:
In ASTDumper
+void VisitConceptTemplateDecl(const ConceptTemplateDecl *D);
Implementation:
void ASTDumper::VisitConceptTemplateDecl(const ConceptTemplateDecl *D) {
dumpName(D);
dumpTemplateParameters(D->getTemplateParameters());
VisitExpr(D->getConstraintExpression());
}
Address the other issues I've mentioned in the inline comments and we're good!
Great job :)
Comment at: include/clang/AST/DeclTemplate.h:3023
+ Expr *ConstraintExpr);
+
+ Expr *getConstraintExpr() const {
Add CreateDeserialized.
Comment at: include/clang/AST/DeclTemplate.h:3027
+ }
+
+ // TODO: Should do source range properly.
Add setConstraintExpr (for use when calling CreateDeserialized, see
ASTDeclReader)
Comment at: include/clang/Parse/Parser.h:2787
AccessSpecifier AS = AS_none);
+ // C++ 17: Template, concept definition [temp]
+ Decl *
C++2a
Comment at: lib/Parse/ParseTemplate.cpp:374
+
+ ExprResult ConstraintExpr = ParseConstraintExpression();
+
Add a check to ParseConstraintExpression that the type is either dependent or
bool, and add an apropriate diagnostic.
```
if (!ConstraintExpr->isTypeDependent() && ConstraintExpr->getType() !=
Context.BoolTy) {
Diag(Init->getSourceRange().getBegin(),
diag::err_concept_initialized_with_non_bool_type) << Init->getType();
Concept->setInvalidDecl();
return;
}
```
Comment at: lib/Sema/SemaTemplate.cpp:2935
if (!Template || isa(Template) ||
isa(Template)) {
// We might have a substituted template template parameter pack. If so,
Add
```
|| isa
```
Comment at: lib/Sema/SemaTemplate.cpp:3936
+ if (R.getAsSingle()) {
+return CheckConceptTemplateId(SS, R.getLookupNameInfo(),
We're gonna want to check
```
!TemplateSpecializationType::anyDependentTemplateArguments(*TemplateArgs,
InstantiationDependent)
```
here as well - so that we can instantiate a ConceptSpecializationExpr later
when we have it (we're not gonna want to instantiate a
ConceptSpecializationExpr with dependent arguments.
Comment at: lib/Sema/SemaTemplate.cpp:7698
+
+ // TODO: Maybe we should check TemplateParameterLists for nullptr?
+ Decl *NewDecl = ConceptDecl::Create(Context, DC, L, Name,
Check that !DC->isFileContext() (Concepts may only be defined at namespace
scope)
Comment at: lib/Sema/SemaTemplate.cpp:7700
+ Decl *NewDecl = ConceptDecl::Create(Context, DC, L, Name,
+ TemplateParameterLists[0],
+ ConstraintExpr);
I think it would be better to just check that TemplateParameterLists.size() !=
1 - it can't happen in a valid situation anyway, you'd want a diagnostic there.
Comment at: lib/Sema/SemaTemplate.cpp:7702
+ ConstraintExpr);
+ ActOnDocumentableDecl(NewDecl);
+ CurContext->addDecl(NewDecl);
Check that c->getAssociatedConstraints() == nullptr ([temp.concept]p4 A concept
shall not have associated constraints).
Add a TODO to make a test for that once we have actual associated constraints.
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:3099
+Decl *TemplateDeclInstantiator::VisitConceptDecl(ConceptDecl *D) {
+ // TODO: Do something here?
+ return nullptr;
Yes -
```
llvm_unreachable("Concept definitions cannot reside inside a template");
```
Comment at: lib/Serialization/ASTReaderDecl.cpp:1975
+void ASTDeclReader::VisitConceptDecl(ConceptDecl *D) {
+ VisitTemplateDecl(D);
+}
Add here:
```
D->setConstraintExpression(Record.readExpr());
```
And in ASTDeclWriter, a method VisitConceptDecl:
```
void ASTDeclWriter::VisitConceptTemplateDecl(ConceptTemplateDecl *D) {
VisitTemplateDecl(D);
Record.AddStmt(D->getConstraintExpression());
Code = serialization::DECL_CONCEPT;
}
```
And call it in the appropriate places.
Comment at: lib/Serialization/ASTReaderDecl.cpp:3503
break;
case DECL_CLASS_SCOPE_FUNCTION_SPECIALIZATION:
D = ClassScopeFunctionSpecializationDecl::CreateDeserialized(Context, ID);
Add a case: DECL_CONCEPT, also create that enum member.
https://reviews.llvm.org/D40381
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
changyu added a comment. I should add that this depends on https://reviews.llvm.org/D40380 https://reviews.llvm.org/D40381 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40381: Parse concept definition
changyu created this revision.
Per p0734r0.
https://reviews.llvm.org/D40381
Files:
include/clang/AST/DeclTemplate.h
include/clang/AST/RecursiveASTVisitor.h
include/clang/Basic/DeclNodes.td
include/clang/Basic/TemplateKinds.h
include/clang/Parse/Parser.h
include/clang/Sema/Sema.h
lib/AST/ASTDumper.cpp
lib/AST/DeclBase.cpp
lib/AST/DeclTemplate.cpp
lib/CodeGen/CGDecl.cpp
lib/Parse/ParseTemplate.cpp
lib/Sema/SemaTemplate.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
lib/Serialization/ASTCommon.cpp
lib/Serialization/ASTReaderDecl.cpp
test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp
test/Parser/cxx-concept-declaration.cpp
tools/libclang/CIndex.cpp
Index: tools/libclang/CIndex.cpp
===
--- tools/libclang/CIndex.cpp
+++ tools/libclang/CIndex.cpp
@@ -5906,6 +5906,7 @@
case Decl::PragmaComment:
case Decl::PragmaDetectMismatch:
case Decl::UsingPack:
+ case Decl::Concept:
return C;
// Declaration kinds that don't make any sense here, but are
Index: test/Parser/cxx-concept-declaration.cpp
===
--- test/Parser/cxx-concept-declaration.cpp
+++ test/Parser/cxx-concept-declaration.cpp
@@ -1,7 +1,7 @@
// Support parsing of concepts
-// Disabled for now.
-// expected-no-diagnostics
// RUN: %clang_cc1 -std=c++14 -fconcepts-ts -x c++ -verify %s
-// template concept C1 = true;
+template concept C1 = true;
+
+template concept D1 = true; // expected-error {{expected template parameter}}
Index: test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp
===
--- /dev/null
+++ test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp
@@ -0,0 +1,5 @@
+// RUN: %clang_cc1 -std=c++1z -fconcepts-ts -fcxx-exceptions -x c++ -verify %s
+// expected-no-diagnostics
+
+template concept C = true;
+static_assert(C);
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -337,6 +337,7 @@
void VisitBindingDecl(BindingDecl *BD);
void VisitNonTypeTemplateParmDecl(NonTypeTemplateParmDecl *D);
DeclID VisitTemplateDecl(TemplateDecl *D);
+void VisitConceptDecl(ConceptDecl *D);
RedeclarableResult VisitRedeclarableTemplateDecl(RedeclarableTemplateDecl *D);
void VisitClassTemplateDecl(ClassTemplateDecl *D);
void VisitBuiltinTemplateDecl(BuiltinTemplateDecl *D);
@@ -1970,6 +1971,10 @@
return PatternID;
}
+void ASTDeclReader::VisitConceptDecl(ConceptDecl *D) {
+ VisitTemplateDecl(D);
+}
+
ASTDeclReader::RedeclarableResult
ASTDeclReader::VisitRedeclarableTemplateDecl(RedeclarableTemplateDecl *D) {
RedeclarableResult Redecl = VisitRedeclarable(D);
Index: lib/Serialization/ASTCommon.cpp
===
--- lib/Serialization/ASTCommon.cpp
+++ lib/Serialization/ASTCommon.cpp
@@ -313,6 +313,7 @@
case Decl::BuiltinTemplate:
case Decl::Decomposition:
case Decl::Binding:
+ case Decl::Concept:
return false;
// These indirectly derive from Redeclarable but are not actually
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -3095,6 +3095,11 @@
return nullptr;
}
+Decl *TemplateDeclInstantiator::VisitConceptDecl(ConceptDecl *D) {
+ // TODO: Do something here?
+ return nullptr;
+}
+
Decl *TemplateDeclInstantiator::VisitDecl(Decl *D) {
llvm_unreachable("Unexpected decl");
}
Index: lib/Sema/SemaTemplate.cpp
===
--- lib/Sema/SemaTemplate.cpp
+++ lib/Sema/SemaTemplate.cpp
@@ -232,9 +232,11 @@
} else {
assert(isa(TD) || isa(TD) ||
isa(TD) || isa(TD) ||
- isa(TD));
+ isa(TD) || isa(TD));
TemplateKind =
- isa(TD) ? TNK_Var_template : TNK_Type_template;
+ isa(TD) ? TNK_Var_template :
+ isa(TD) ? TNK_Concept_template :
+ TNK_Type_template;
}
}
@@ -3884,6 +3886,24 @@
/*FoundD=*/nullptr, TemplateArgs);
}
+ExprResult
+Sema::CheckConceptTemplateId(const CXXScopeSpec ,
+ const DeclarationNameInfo ,
+ ConceptDecl *Template, SourceLocation TemplateLoc,
+ const TemplateArgumentListInfo *TemplateArgs) {
+
+ // DeclResult Decl = CheckVarTemplateId(Template, TemplateLoc, NameInfo.getLoc(),
+ // *TemplateArgs);
+ // if (Decl.isInvalid())
+ // return ExprError();
+
+ // // Build an ordinary singleton decl ref.
+ // return BuildDeclarationNameExpr(SS, NameInfo, cast(Decl.get()),
+ //
