[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D126907#3615807 <https://reviews.llvm.org/D126907#3615807>, @erichkeane wrote:

> All tests pass now, I was able to get the template-template checks working correctly, and it passes all the tests I have available.  @ChuanqiXu if you could review/run what tests you have, it would be greatly appreciated.

I've tested some of our workloads and libunifex (I know it contains a lot of uses for concept). And all the tests passed now. So it looks like it wouldn't cause regression failure.

The implementation **basically** looks good to me. (This is really large and I can't be sure I don't miss something). Only some minor issues remained.



================
Comment at: clang/include/clang/AST/Decl.h:1944-1948
+  /// For non-templates this value will be NULL, unless this non-template
+  /// function declaration was declared directly inside of a function template,
+  /// in which case this will have a pointer to a FunctionDecl, stored in the
+  /// NamedDecl. For function declarations that describe a function template,
+  /// this will be a pointer to a FunctionTemplateDecl, stored in the NamedDecl.
----------------
Could we give a new abstract like `XXXInfo` to replace `NamedDecl*` here? The NamedDecl* covers a lot of things. It looks more consistent.


================
Comment at: clang/include/clang/Sema/Sema.h:7056-7067
+  /*
+  // Keep track of whether we are evaluating a constraint.
+  unsigned ConstraintEvaluationDepth = 0;
+
+  class ConstraintEvalRAII {
+    Sema &S;
+
----------------
Remove this before landing.


================
Comment at: clang/include/clang/Sema/Sema.h:7070
 public:
+  // bool IsEvaluatingAConstraint() { return ConstraintEvaluationDepth > 0; }
   const NormalizedConstraint *
----------------
ditto


================
Comment at: clang/lib/AST/Decl.cpp:3788
          "Member function is already a specialization");
-  TemplateOrSpecialization = Template;
+  TemplateOrSpecialization = static_cast<NamedDecl *>(Template);
+}
----------------
`static_cast` is rarely used in clang/LLVM to me. I know the reason is that `TemplateOrSpecialization` contains a `NamedDecl*` member. I guess it might be better to refactor it.


================
Comment at: clang/lib/AST/DeclBase.cpp:286-304
 const DeclContext *Decl::getParentFunctionOrMethod() const {
   for (const DeclContext *DC = getDeclContext();
        DC && !DC->isTranslationUnit() && !DC->isNamespace();
        DC = DC->getParent())
     if (DC->isFunctionOrMethod())
       return DC;
 
----------------
How about:


================
Comment at: clang/lib/Sema/SemaConcept.cpp:63
+
+  ExprResult recreateBinOp(Sema &SemaRef, ExprResult LHS) {
+    return recreateBinOp(SemaRef, LHS, const_cast<Expr *>(getRHS()));
----------------



================
Comment at: clang/lib/Sema/SemaConcept.cpp:67
+
+  ExprResult recreateBinOp(Sema &SemaRef, ExprResult LHS, ExprResult RHS) {
+    assert((isAnd() || isOr()) && "Not the right kind of op?");
----------------



================
Comment at: clang/lib/Sema/SemaConcept.cpp:186
+    return BO.recreateBinOp(S, LHSRes, RHSRes);
   } else if (auto *C = dyn_cast<ExprWithCleanups>(ConstraintExpr)) {
+    // These aren't evaluated, so we don't care about cleanups, so we can just
----------------



================
Comment at: clang/lib/Sema/SemaTemplate.cpp:2339
       if (const auto *TC = TTP->getTypeConstraint())
-        SemaRef.SubstTypeConstraint(NewTTP, TC, Args);
+        SemaRef.SubstTypeConstraint(NewTTP, TC, Args, false);
       if (TTP->hasDefaultArgument()) {
----------------



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

https://reviews.llvm.org/D126907