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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
