[PATCH] D41217: [Concepts] Concept Specialization Expressions

[Saar Raz via Phabricator via cfe-commits]

saar.raz added a comment.

@rsmith - thanks for the responses!
Will also address the comments I haven't responded to soon.



================
Comment at: include/clang/AST/ExprCXX.h:4420
+  /// \brief The concept named.
+  ConceptDecl *NamedConcept;
+
----------------
rsmith wrote:
> 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.
Isn't the `FoundDecl` just the `NamedConcept`?


================
Comment at: include/clang/AST/ExprCXX.h:4423-4424
+  /// \brief The template argument list used to specialize the concept.
+  TemplateArgumentList *TemplateArgs;
+  const ASTTemplateArgumentListInfo *TemplateArgInfo;
+
----------------
rsmith wrote:
> It'd be nice if at least one of these two arrays could be tail-allocated.
Now that you mention it, TemplateArgs is never even used or set, I'll remove it.
As for TemplateArgInfo - do you mean allocate the ASTTemplateArgumentListInfo itself as a trailing obj or the individual TemplateArgumentLocs? the former seems more reasonable to me as it's convenient to have an ASTTemplateArgumentListInfo object around


================
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;
----------------
rsmith wrote:
> Do you really need mutators for the 'satisfied' flag and the concept name location?
They're here only because of ASTReader/Writer. I guess I can use a friend decl instead.


================
Comment at: lib/AST/StmtPrinter.cpp:2553
+void StmtPrinter::VisitConceptSpecializationExpr(ConceptSpecializationExpr *E) {
+  OS << E->getNamedConcept()->getName();
+  printTemplateArgumentList(OS, E->getTemplateArgumentListInfo()->arguments(),
----------------
rsmith wrote:
> 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).
Are there possible upcoming changes that'll make them differ?


================
Comment at: lib/Sema/SemaConcept.cpp:34
+      Diag(ConstraintExpression->getExprLoc(),
+           diag::err_non_bool_atomic_constraint)
+              << ConstraintExpression << ConstraintExpression->getType();
----------------
rsmith wrote:
> 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.)
I guess this is already justified, if awkwardly (NDR) by [temp.res]p8.2


Repository:
  rC Clang

https://reviews.llvm.org/D41217