[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 8 inline comments as done.
erichkeane added inline comments.


================
Comment at: clang/include/clang/Sema/Sema.h:3642
+  // template, for the purposes of [temp.friend] p9.
+  bool ConstraintExpressionDependsOnEnclosingTemplate(unsigned TemplateDepth,
+                                                      const Expr *Constraint);
----------------
ChuanqiXu wrote:
> The meaning of `TemplateDepth` is unclear. Do it mean top-down or start from the constraint expression?
It is top-down, as the constraint is uninstantiated.  I've added a comment to clarify.


================
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;
----------------
ChuanqiXu wrote:
> Could we remove the duplicates? For example, is it possible to make ConstraintEvalRAII a subclass of Sema?
It ends up having to be a template, and for a 'getter' to exist, but done.


================
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) {
----------------
ChuanqiXu wrote:
> nit:
These are specializations, so they cant have a storage class.  They inherit them from the primary template.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:1233
                         /* DeclContext *Owner */ Owner, TemplateArgs);
+      DeclInstantiator.setEvaluateConstraints(EvaluateConstraints);
+      return DeclInstantiator.SubstTemplateParams(OrigTPL);
----------------
ChuanqiXu wrote:
> 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);
>    ...
> }
> ```
Turns out I don't think this was required anyway, so I just removed it.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:2435
+  CXXRecordDecl *Record = cast<CXXRecordDecl>(DC);
+  Expr *TrailingRequiresClause = D->getTrailingRequiresClause();
+
----------------
ChuanqiXu wrote:
> Is this used?
Yes, see line 2453, 2459, 2470, and 2476.


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

https://reviews.llvm.org/D126907