rsmith added inline comments.
================
Comment at: lib/AST/ItaniumMangle.cpp:4127
+
+ case Expr::ConceptSpecializationExprClass: {
+ // <expr-primary> ::= L <mangled-name> E # external name
----------------
These mangling changes look like they could be separated out from the rest of
the patch. These plus the associated test look fine to check in as-is.
================
Comment at: lib/Sema/SemaConcept.cpp:161
+ DiagString = ": ";
+ SubstDiag.second.EmitToString(S.getDiagnostics(), DiagString);
+ Satisfaction.Details.emplace_back(
----------------
We should store the diagnostic as a `PartialDiagnostic` rather than as a
string. (Currently that's a pain: we don't have serialization support for
`PartialDiagnostic`s, and they have a huge storage cost -- probably even larger
than the fully-formatted string. But it's the right thing to do in the longer
term. We want to expose the structured diagnostic to tools (such as IDEs) that
want to present them in different ways, and if we ever support translation of
diagnostics, we don't want the contents of module files to depend on the
diagnostic language used when building the module file.)
Please add a FIXME for this.
================
Comment at: lib/Sema/SemaDeclCXX.cpp:14032
std::tie(InnerCond, InnerCondDescription) =
findFailedBooleanCondition(Converted.get());
+ if (InnerCond && isa<ConceptSpecializationExpr>(InnerCond)) {
----------------
Would it be reasonable and feasible to unify `findFailedBooleanCondition` and
`diagnoseWellFormedUnsatisfiedConstraintExpr`? They seem to be doing quite
similar things.
================
Comment at: lib/Sema/SemaTemplate.cpp:7795-7797
+ if (!InstantiationDependent
+ && EnsureTemplateArgumentListConstraints(ClassTemplate, Converted,
+ SourceRange(TemplateNameLoc, RAngleLoc)))
----------------
Can we move this into `CheckTemplateArgumentList`? Would that make sense in the
bigger picture?
(Please put the `&&` on the previous line.)
================
Comment at: lib/Sema/SemaTemplateDeduction.cpp:2838
+ ArrayRef<TemplateArgument> DeducedArgs,
+ TemplateDeductionInfo& Info) {
+ llvm::SmallVector<const Expr *, 3> AssociatedConstraints;
----------------
Style nit: space before `&` not after
================
Comment at: lib/Sema/SemaTemplateDeduction.cpp:2844
+ Info.AssociatedConstraintsSatisfaction)
+ || !Info.AssociatedConstraintsSatisfaction.IsSatisfied) {
+ Info.reset(TemplateArgumentList::CreateCopy(S.Context, DeducedArgs));
----------------
Style nit: `||` on previous line
================
Comment at: lib/Sema/SemaTemplateDeduction.cpp:2890-2892
+ if (TemplateDeductionResult Result
+ = CheckDeducedArgumentConstraints(*this, Partial, DeducedArgs, Info))
+ return Result;
----------------
This should be done in (or maybe immediately after)
`ConvertDeducedTemplateArguments` (called by
`FinishTemplateArgumentDeduction`), rather than here: that way, we'll convert
the deduced arguments to the right kind, and pick up (and substitute into) any
default template arguments as necessary before checking constraint
satisfaction. We probably typically get away with this for now because partial
specializations can't have default arguments, but we really should be forming
the complete finalized argument list before checking satisfaction.
================
Comment at: lib/Sema/SemaTemplateDeduction.cpp:2935-2938
+ if (TemplateDeductionResult Result
+ = CheckDeducedArgumentConstraints(*this, Partial, DeducedArgs, Info))
+ return Result;
+
----------------
Likewise.
================
Comment at: lib/Sema/SemaTemplateDeduction.cpp:3049-3052
+ = CheckDeducedArgumentConstraints(*this, FunctionTemplate,
+ ExplicitArgumentList->asArray(),
+ Info))
+ return Result;
----------------
Is this check justified by some wording in the standard? Checking a dependent
set of constraints (before we've done any deduction) seems surprising.
(I asked this on the core reflector, and it sounds like the status quo answer
is no: we should not check constraints before we have all the template
arguments for a function template. I'm not convinced that's really the right
answer going forward, but we can at least run with it for the time being, to
collect evidence to support a different rule if nothing else.)
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D41569/new/
https://reviews.llvm.org/D41569
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
