[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 9 inline comments as done.
erichkeane added a comment.

In D126907#3617713 <https://reviews.llvm.org/D126907#3617713>, @ChuanqiXu wrote:

> 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.

Thank you SO much for your testing and reviews, I very much appreciate all the help you've given me.  I'm sorry that this has become such a huge patch, I really wish it was something I could have done in smaller increments, but that doesn't seem like it is possible since it is changing something so fundamental, and the fallout from that is huge.



================
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.
----------------
ChuanqiXu wrote:
> Could we give a new abstract like `XXXInfo` to replace `NamedDecl*` here? The NamedDecl* covers a lot of things. It looks more consistent.
All of those 'Info' types contain significantly more information, which we really don't have a need for here.  Basically all this patch is doing is using the FunctionTemplateDecl* that was already in the list and generalizing it a bit.  AND unfortunately, the only commonality between FunctionTemplateDecl and FunctionDecl is NamedDecl.  

So any type we would use would end up causing an additional allocation here, and make its use require us to unpack it :/

I guess what I'm saying, is I'm not sure what that would look like in a way that wouldn't make this worse.  Do you have an idea you could share that would improve that?  




================
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;
+
----------------
ChuanqiXu wrote:
> Remove this before landing.
Yikes!  Thanks for catching this!


================
Comment at: clang/lib/AST/Decl.cpp:3788
          "Member function is already a specialization");
-  TemplateOrSpecialization = Template;
+  TemplateOrSpecialization = static_cast<NamedDecl *>(Template);
+}
----------------
ChuanqiXu wrote:
> `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.
Ah, looks like the static-casts weren't necessary anyway.  No idea why I thought they were.  Removed.  See comment above about the refactor.


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

https://reviews.llvm.org/D126907