rsmith added inline comments.
================
Comment at: include/clang/AST/ExprCXX.h:4420
+ /// \brief The concept named.
+ ConceptDecl *NamedConcept;
+
----------------
saar.raz wrote:
> 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`?
Not in general, no. The `FoundDecl` could be a `UsingShadowDecl`.
================
Comment at: lib/AST/ExprCXX.cpp:1472
+ for (const TemplateArgumentLoc& LocInfo : ArgsAsWritten->arguments()) {
+ if (LocInfo.getArgument().isInstantiationDependent()) {
+ IsDependent = true;
----------------
This is incorrect. Whether the expression is dependent is not the same thing as
whether it's instantiation-depnedent. A concept specialization should be
considered instantiation-dependent if it has any instantiation-dependent
template arguments, and should be considered value-dependent if it has any
dependent template arguments.
For example, `Concept<sizeof(sizeof(T))>` has an instantiation-dependent
template argument, but no dependent template arguments, so should be
instantiation-dependent (the validity of the construct can depend on the type
`T`) but not value-dependent (the value of the construct is always
`Concept<sizeof(size_t)>`, which doesn't depend on `T`).
================
Comment at: lib/AST/StmtPrinter.cpp:2553
+void StmtPrinter::VisitConceptSpecializationExpr(ConceptSpecializationExpr *E)
{
+ OS << E->getNamedConcept()->getName();
+ printTemplateArgumentList(OS, E->getTemplateArgumentListInfo()->arguments(),
----------------
saar.raz wrote:
> 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?
p0945r0 would allow them to differ.
================
Comment at: lib/Sema/SemaConcept.cpp:105-112
+ if (!E.get()->EvaluateAsBooleanCondition(IsSatisfied, Context)) {
+ // C++2a [temp.constr.atomic]p1
+ // ...E shall be a constant expression of type bool.
+ Diag(E.get()->getLocStart(),
+ diag::err_non_constant_constraint_expression)
+ << E.get()->getSourceRange();
+ return true;
----------------
Please collect the notes from `EvalutaeAsBooleanCondition` and emit them here,
so the user knows /why/ the expression was non-constant.
================
Comment at: lib/Sema/SemaTemplate.cpp:3907-3911
+ for (TemplateArgument &Arg : Converted)
+ if (Arg.isInstantiationDependent()) {
+ IsDependent = true;
+ break;
+ }
----------------
Add braces around this `for`, because its body contains multiple statements.
(Generally, if an inner construct has braces we want outer constructs to have
them too.)
================
Comment at: lib/Sema/SemaTemplateInstantiate.cpp:679-681
+ Diags.Report(Active->PointOfInstantiation,
+ diag::note_constraint_substitution_here)
+ << Active->InstantiationRange;
----------------
Is this note ever useful? It will presumably always point into the same concept
definition that the prior diagnostic also pointed at, and doesn't seem to add
anything in the testcases.
Maybe we could keep the CodeSynthesisContext around as a marker that we've
entered a SFINAE context, but not have any corresponding diagnostic. (The note
produced for the enclosing `ConstraintsCheck` context covers that.) Or we could
remove this and store a flag on the `ConstraintsCheck` to indicate whether
we're in a SFINAEable portion of it.
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
