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<ConceptDecl>`, 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?">;
+def err_concept_extra_headers : Error<
----------------
Drop the ", did you attempt this?". It doesn't add anything to the diagnostic,
and might be read as a little snarky, frustrating the user.
================
Comment at: lib/AST/ASTDumper.cpp:182-186
+void ASTDumper::VisitConceptDecl(const ConceptDecl *D) {
+ NodeDumper.dumpName(D);
+ dumpTemplateParameters(D->getTemplateParameters());
+ Visit(D->getConstraintExpr());
+}
----------------
This needs rebasing; I believe the right place for this is now split between
TextNodeDumper.cpp and ASTNodeTraverser.h.
================
Comment at: lib/AST/DeclPrinter.cpp:1084-1085
Out << D->getName();
- } else {
- Visit(D->getTemplatedDecl());
- }
+ } else if (auto *TD = D->getTemplatedDecl())
+ Visit(TD);
}
----------------
This looks wrong: we'll only print the template parameters, and not the
`concept <name> = <expr>;` part. Please add another `else if` for the
`ConceptDecl` case (or similar).
================
Comment at: lib/Index/IndexDecl.cpp:658-662
+ if (isa<ConceptDecl>(TD))
+ // Concepts are not redeclarable.
+ return true;
+ if (!D)
+ return false;
----------------
This change looks redundant. Note that `VisitTemplateDecl` below already
returned `true` if `getTemplatedDecl()` is null.
================
Comment at: lib/Parse/ParseTemplate.cpp:58
+/// template-head: [C++2a]
+/// 'export'[opt] 'template' '<' template-parameter-list '>'
+/// requires-clause[opt]
----------------
There's no 'export'[opt] in //template-head//.
================
Comment at: lib/Parse/ParseTemplate.cpp:387
+
+ if (!TryConsumeToken(tok::equal)) {
+ Diag(Tok.getLocation(), diag::err_expected) << tok::equal;
----------------
Consider calling `DiagnoseAndSkipCXX11Attributes` before and after parsing the
name of the concept; it seems likely that people will try adding attributes
there, and we should give a good diagnostic for this until they start being
permitted in one of those two locations.
================
Comment at: lib/Sema/SemaTemplate.cpp:7928
+ ActOnDocumentableDecl(NewDecl);
+ CurContext->addDecl(NewDecl);
+ return NewDecl;
----------------
This should be `PushOnScopeChains(NewDecl, S);` instead (though I think in
practice it won't matter until/unless we start supporting block-scope concept
definitions).
================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:3365
+ llvm_unreachable("Concept definitions cannot reside inside a template");
+ return nullptr;
+}
----------------
No need for a `return` here; the code after an `llvm_unreachable` is
unreachable =)
================
Comment at: lib/Serialization/ASTReaderDecl.cpp:2082
+ D->ConstraintExpr = Record.readExpr();
+}
+
----------------
Call `mergeMergeable(D)` at the end of 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
