[PATCH] D119544: Deferred Concept Instantiation Implementation

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

Oh, I'm busy with coroutines these days. Thanks for reminding me for this. LGTM to me basically. Only comments for style left.



================
Comment at: clang/lib/Sema/SemaConcept.cpp:70
+    assert((!LHS.isInvalid() && !RHS.isInvalid()) && "not good expressions?");
+    assert(LHS.isUsable() && RHS.isUsable() && "Side not usable?");
+    // We should just be able to 'normalize' these to the builtin Binary
----------------
I feel like the usage of the API could be further simplified.


================
Comment at: clang/lib/Sema/SemaConcept.cpp:338-340
+      // Backfill the 'converted' list with nulls so we can keep the Converted
+      Converted.append(ConstraintExprs.size() - Converted.size(), nullptr);
+      // and unconverted lists in sync. [temp.constr.op] p2
----------------



================
Comment at: clang/lib/Sema/SemaConcept.cpp:365-370
+  for (unsigned Depth = 0; Depth < TemplateArgsList.getNumSubstitutedLevels();
+       ++Depth) {
+    for (unsigned Index = 0;
+         Index < TemplateArgsList.getNumSubstitutedArgs(Depth); ++Index)
+      FlattenedArgs.push_back(TemplateArgsList(Depth, Index));
+  }
----------------
I think it is a chance to add range style interfaces for MultiLevelTemplateArgumentList. So that we could use range-based loop here.


================
Comment at: clang/lib/Sema/SemaConcept.cpp:432-476
+    if (TemplateArgs) {
+      MultiLevelTemplateArgumentList JustTemplArgs(
+          *FD->getTemplateSpecializationArgs());
+      if (addInstantiatedParametersToScope(
+              FD, PrimaryTemplate->getTemplatedDecl(), Scope, JustTemplArgs))
+        return true;
+    }
----------------
The suggested change works. I feel it is not identical with the original. Is it correct or do we miss something in the test?


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:2173
     Function->setInstantiationOfMemberFunction(D, TSK_ImplicitInstantiation);
+  } else if (!isFriend) {
+    // If this is not a function template, and this is not a friend (that is,
----------------
The check here is not straightforward. Here we want to check if the Function is local declared function. But what we check here is about friendness. I am not 100% sure if it is right and other reader might be confusing too. I would suggest to check it directly. e.g, check if one of its DeclContext is FunctionDecl.


================
Comment at: clang/lib/Sema/TreeTransform.h:13003
   if (Expr *TRC = E->getCallOperator()->getTrailingRequiresClause())
-    // FIXME: Concepts: Substitution into requires clause should only happen
-    //                  when checking satisfaction.
-    NewTrailingRequiresClause = getDerived().TransformExpr(TRC);
+    NewTrailingRequiresClause = TRC;
 
----------------
I think we could eliminate `NewTrailingRequiresClause`


================
Comment at: clang/test/SemaTemplate/concepts.cpp:275-276
+template <typename T>
+constexpr bool constraint = PR54443::is_same<typename remove_ref<T>::type,
+                                             int>::value;
+
----------------
Better to rename `constraint` to something like `IsInt`


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

https://reviews.llvm.org/D119544