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