[PATCH] D41217: [Concepts] Concept Specialization Expressions
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Aug 6 17:35:16 PDT 2018
rsmith added inline comments.
================
Comment at: include/clang/AST/ExprCXX.h:4420
+ /// \brief The concept named.
+ ConceptDecl *NamedConcept;
+
----------------
You should also track the `FoundDecl` and the optional `NestedNameSpecifierLoc` (just like a `DeclRefExpr` would). Clang-based tools (particularly refactoring tools) need those. There's also an optional `template` keyword, but it can never matter in practice because the nested name specifier can never be dependent, so it's probably not the most high-priority thing to track.
================
Comment at: include/clang/AST/ExprCXX.h:4423-4424
+ /// \brief The template argument list used to specialize the concept.
+ TemplateArgumentList *TemplateArgs;
+ const ASTTemplateArgumentListInfo *TemplateArgInfo;
+
----------------
It'd be nice if at least one of these two arrays could be tail-allocated.
================
Comment at: include/clang/AST/ExprCXX.h:4459-4463
+ /// \brief Set new template arguments for this concept specialization. Returns
+ /// true if an error occured (the template arguments do not match the concept,
+ /// probably)
+ bool setTemplateArguments(ASTContext &C, Sema *S,
+ const TemplateArgumentListInfo *TALI);
----------------
Passing `Sema` in here is a major layering violation. Here's how this should work:
* The AST code represents the AST model; its functions should generally not be able to fail.
* Sema checks the semantic constraints and enforces the validity of operations, and updates the AST node as appropriate
In this case, Sema should be checking the template argument list as written against the concept's parameter list, forming a resolved list of template arguments, and then passing both the as-written information and the resolved arguments to this AST node for storage.
================
Comment at: include/clang/AST/ExprCXX.h:4474-4481
+ void setSatisfied(bool Satisfied) {
+ IsSatisfied = Satisfied;
+ }
+
+ SourceLocation getConceptNameLoc() const { return ConceptNameLoc; }
+ void setConceptNameLoc(SourceLocation Loc) {
+ ConceptNameLoc = Loc;
----------------
Do you really need mutators for the 'satisfied' flag and the concept name location?
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2405
+ "substitution into constraint expression resulted in a non-constant "
+ "expression '%0'">;
+def err_non_bool_atomic_constraint : Error<
----------------
Find a way to describe this problem without pretty-printing a substituted expression.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2407
+def err_non_bool_atomic_constraint : Error<
+ "atomic constraint '%0' must be of type 'bool' (found %1)">;
+def note_in_concept_specialization : Note<
----------------
Remove the '%0' here; you're already underlining it with a `SourceRange` (right?).
Please read http://clang.llvm.org/docs/InternalsManual.html#the-format-string and in particular "Take advantage of location information. The user will be able to see the line and location of the caret, so you don’t need to tell them that the problem is with the 4th argument to the function: just point to it."
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2409
+def note_in_concept_specialization : Note<
+ "in concept specialization '%0'">;
----------------
There's not really any such thing as a "concept specialization". Perhaps "while checking satisfaction of '%0'" or similar?
================
Comment at: lib/AST/ExprCXX.cpp:1460
+ Expr *ConstraintExpr, bool &IsSatisfied) {
+ if (auto *BO = dyn_cast<BinaryOperator>(ConstraintExpr)) {
+ if (BO->getOpcode() == BO_LAnd) {
----------------
You will need to skip over a top-level `ExprWithCleanups` (I can't think off-hand of any other nodes that could show up here, but there might be more).
================
Comment at: lib/AST/ExprCXX.cpp:1552-1553
+ IsSatisfied)) {
+ S->Diag(getLocStart(),
+ diag::note_in_concept_specialization) << this;
+ return true;
----------------
Build a `CodeSynthesisContext` for this rather than adding an ad-hoc note to the last diagnostic that happens to come from substitution (which might not be the error you care about, and might even be suppressed by warning flags).
================
Comment at: lib/AST/ExprConstant.cpp:9085-9087
+ if (E->isValueDependent()) {
+ return Error(E);
+ }
----------------
You don't need to check for this. If the evaluator is called on a value-dependent expression, that's a bug in the caller.
================
Comment at: lib/AST/StmtPrinter.cpp:2553
+void StmtPrinter::VisitConceptSpecializationExpr(ConceptSpecializationExpr *E) {
+ OS << E->getNamedConcept()->getName();
+ printTemplateArgumentList(OS, E->getTemplateArgumentListInfo()->arguments(),
----------------
You should print out the nested name specifier here. (And ideally also the `template` keyword, if specified...). And you should print the name of the found declaration, not the name of the concept (although they can't differ currently).
================
Comment at: lib/Parse/ParseTemplate.cpp:374-377
if (ConstraintExprResult.isInvalid()) {
- Diag(Tok.getLocation(), diag::err_expected_expression)
- << "constraint-expression";
+ // ParseConstraintExpression will have given a sufficient diagnostic.
return nullptr;
}
----------------
Remove comment and braces. We don't need to say that an invalid `ExprResult` means a diagnostic was produced, because that's always implied by the `ExprResult` being invalid.
================
Comment at: lib/Sema/SemaConcept.cpp:22
+
+ if (BinaryOperator* BinOp = dyn_cast<BinaryOperator>(ConstraintExpression)) {
+ if (BinOp->getOpcode() == BO_LAnd || BinOp->getOpcode() == BO_LOr) {
----------------
`*` on the right, please. And replace `BinaryOperator` with `auto` since it's initialized by a `dyn_cast`.
================
Comment at: lib/Sema/SemaConcept.cpp:24-25
+ if (BinOp->getOpcode() == BO_LAnd || BinOp->getOpcode() == BO_LOr) {
+ return CheckConstraintExpression(BinOp->getLHS())
+ && CheckConstraintExpression(BinOp->getRHS());
+ }
----------------
`&&` on the previous line, and align the two operands. (Or get clang-format to do it for you.)
================
Comment at: lib/Sema/SemaConcept.cpp:34
+ Diag(ConstraintExpression->getExprLoc(),
+ diag::err_non_bool_atomic_constraint)
+ << ConstraintExpression << ConstraintExpression->getType();
----------------
What justifies rejecting this prior to any use of the concept that would result in a satisfaction check?
(I think checking this is a good thing; what I'm wondering is whether we need to file a core issue to get the wording updated to allow us to reject such bogus concepts even if they're never used.)
================
Comment at: lib/Sema/SemaConcept.cpp:37-41
+ } else if (ImplicitCastExpr *E =
+ dyn_cast<ImplicitCastExpr>(ConstraintExpression)) {
+ // This will catch expressions such as '2 && true'
+ return CheckConstraintExpression(E->getSubExpr());
+ }
----------------
Call `IgnoreParenImpCasts` before checking the type of an atomic constraint, rather than adding an extra recursive step here.
================
Comment at: lib/Sema/SemaConcept.cpp:45
+}
\ No newline at end of file
----------------
Please fix :)
================
Comment at: lib/Sema/SemaExprCXX.cpp:7690
+}
\ No newline at end of file
----------------
Please fix.
================
Comment at: lib/Sema/TreeTransform.h:2943-2956
+ /// \brief Build a new concept specialization expression.
+ ///
+ /// By default, performs semantic analysis to build the new expression.
+ /// Subclasses may override this routine to provide different behavior.
+ ExprResult RebuildConceptSpecializationExpr(SourceLocation ConceptNameLoc,
+ ConceptDecl *CTD,
+ TemplateArgumentListInfo *TALI) {
----------------
We need to convert the template arguments when rebuilding. This should call `CheckConceptTemplateId` instead. (After fixing that, please inline `CreateConceptSpecializationExpr` into its only caller.)
Please also add a test that checks we do convert the template arguments here.
Repository:
rC Clang
https://reviews.llvm.org/D41217
More information about the cfe-commits
mailing list