[PATCH] D41569: [Concepts] Constraint enforcement and diagnostics

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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