[PATCH] D156027: [clang][Interp] Rework how initializers work

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D156027#4544346 <https://reviews.llvm.org/D156027#4544346>, @tbaeder wrote:

> Do you want me to squash the patches I abandoned into this one?

I think that could help us to see the bigger picture; I'm assuming that only adds a bit of code rather than three full patches' worth of code.



================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:240
+    return Initializing ? this->visitInitializer(SubExpr)
+                        : this->visit(SubExpr);
 
----------------
tbaeder wrote:
> aaron.ballman wrote:
> > tbaeder wrote:
> > > This pattern shows up a few times, so it might make sense to add a function that simply passes on all the flags and doesn't do anything else. I'm just not sure what to call it.
> > It's not clear to me when you should use this pattern to begin with.
> Whenever you just pass on the visit, essentially ignoring the current node. Another good example is `ConstantExpr`s, where we don't do anything for the node and then just pass move on to the `SubExpr`.
Ah, I see, so it's basically when you want to delegate the visit to a child node? If so, perhaps that's a reasonable name (`delegateVisit()`)?


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:2090
+      assert(Initializing);
+      if (!isa<CXXMemberCallExpr>(E)) {
+        if (!this->emitDupPtr(E))
----------------
tbaeder wrote:
> aaron.ballman wrote:
> > And if it is a member call expression?
> For member calls, the layout at this point is:
> 
> ```
> ThisPtr
> RVOPtr
> RVOPtr
> ```
> (top of the stack is where we're at). The dup here is supposed to dup the RVO ptr, but we can't do that for member calls because the top is currently the instance pointer. Tha'ts why `VisitCXXMemberCallExpr()` handles this separately.
Thank you for the explanation!


================
Comment at: clang/lib/AST/Interp/Context.cpp:131
   if (T->isFunctionPointerType() || T->isFunctionReferenceType() ||
-      T->isFunctionType())
+      T->isFunctionType() || T->isSpecificBuiltinType(BuiltinType::BoundMember))
     return PT_FnPtr;
----------------
tbaeder wrote:
> aaron.ballman wrote:
> > tbaeder wrote:
> > > I've removed this change in https://reviews.llvm.org/D144164 since it didn't  seem necessary, but it //is// necessary after applying this patch.
> > Which test case exercises this bit?
> We have this line in `records.cpp`:
> 
> ```
>   constexpr int value = (s.*foo)();
> ```
> and its type is:
> 
> ```
> BuiltinType 0x62100001fbe0 '<bound member function type>'
> ```
> 
> it just so happens that we _now_ call `classify()` on this earlier in the code path while before we didn't do that, so we need to know that the bound member function type is now some composite type.
Ah, thank you! That makes sense.


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

https://reviews.llvm.org/D156027