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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
