[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

I am not sure if I don't miss any points. But if it is OK to run libc++/libstdc++, I think it should be good to land this.



================
Comment at: clang/include/clang/Sema/Sema.h:3642
+  // template, for the purposes of [temp.friend] p9.
+  bool ConstraintExpressionDependsOnEnclosingTemplate(unsigned TemplateDepth,
+                                                      const Expr *Constraint);
----------------
The meaning of `TemplateDepth` is unclear. Do it mean top-down or start from the constraint expression?


================
Comment at: clang/include/clang/Sema/Template.h:507-518
+    struct ConstraintEvalRAII {
+      TemplateDeclInstantiator &TI;
+      bool OldValue;
+
+      ConstraintEvalRAII(TemplateDeclInstantiator &TI)
+          : TI(TI), OldValue(TI.EvaluateConstraints) {
+        TI.EvaluateConstraints = false;
----------------
Could we remove the duplicates? For example, is it possible to make ConstraintEvalRAII a subclass of Sema?


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2786-2799
+template <typename TemplateDeclT>
+static bool DeducedArgsNeedReplacement(TemplateDeclT *Template) {
+  return false;
+}
+template <>
+bool DeducedArgsNeedReplacement<VarTemplatePartialSpecializationDecl>(
+    VarTemplatePartialSpecializationDecl *Spec) {
----------------
nit:


================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:1233
                         /* DeclContext *Owner */ Owner, TemplateArgs);
+      DeclInstantiator.setEvaluateConstraints(EvaluateConstraints);
+      return DeclInstantiator.SubstTemplateParams(OrigTPL);
----------------
Do we have any method to detect if the template parameter list lives in a require clause or not? The current duplicating looks a little bit bad.

If there is no such methods, I guess it may be better by using enums:

```
TemplateParameterList *TransformTemplateParameterList(
                              TemplateParameterList *OrigTPL, enum IsInRequire = Normal) {
   ...
   if (IsInRequire == ...)
      DeclInstantiator.setEvaluateConstraints(EvaluateConstraints);
   ...
}
```


================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:3609-3610
+
+  // TODO: ERICH: This is where we need to make sure we 'know' constraint
+  // checking needs to happen.
+  TemplateInstantiator Instantiator(*this, TemplateArgs, SourceLocation(),
----------------



================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:2435
+  CXXRecordDecl *Record = cast<CXXRecordDecl>(DC);
+  Expr *TrailingRequiresClause = D->getTrailingRequiresClause();
+
----------------
Is this used?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126907/new/

https://reviews.llvm.org/D126907