[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

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


================
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
----------------
ChuanqiXu wrote:
> I feel like the usage of the API could be further simplified.
Heh, the suggestion is inverted slightly (those should be `!isUsable`) which caught me for a while :)  Either way, I think it is a good suggestion.


================
Comment at: clang/lib/Sema/SemaConcept.cpp:432-476
+    if (TemplateArgs) {
+      MultiLevelTemplateArgumentList JustTemplArgs(
+          *FD->getTemplateSpecializationArgs());
+      if (addInstantiatedParametersToScope(
+              FD, PrimaryTemplate->getTemplatedDecl(), Scope, JustTemplArgs))
+        return true;
+    }
----------------
ChuanqiXu wrote:
> The suggested change works. I feel it is not identical with the original. Is it correct or do we miss something in the test?
The very top condition is not exactly the same I think, but in a way that I don't believe matters now that this function isn't recursive.

Otherwise I think this is a vast improvement, so I'll keep it!


================
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,
----------------
ChuanqiXu wrote:
> 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.
I cannot come up with a better one here.  Really the condition here is "this is not one of the above conditions, and is still not a friend function".  A friend could ALSO have a `DeclContext` of a `FunctionDecl` as well, so I don't have a good idea of what we could do.


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

https://reviews.llvm.org/D119544