[PATCH] D134699: [clang][Interp] Implement This pointer passing to methods

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/AST/Interp/ByteCodeEmitter.cpp:47
+  bool HasThisPointer = false;
+  if (const auto *MD = dyn_cast<CXXMethodDecl>(F); MD && !MD->isStatic()) {
+    HasThisPointer = true;
----------------



================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:638
   if (const auto CtorExpr = dyn_cast<CXXConstructExpr>(Initializer)) {
-    const CXXConstructorDecl *Ctor = CtorExpr->getConstructor();
-    const RecordDecl *RD = Ctor->getParent();
-    const Record *R = getRecord(RD);
-
-    for (const auto *Init : Ctor->inits()) {
-      const FieldDecl *Member = Init->getMember();
-      const Expr *InitExpr = Init->getInit();
-
-      if (Optional<PrimType> T = classify(InitExpr->getType())) {
-        const Record::Field *F = R->getField(Member);
-
-        if (!this->emitDupPtr(Initializer))
-          return false;
+    const Function *Func = getFunction(CtorExpr->getConstructor());
 
----------------
What happens if `Func` is null?


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:668
     const Decl *Callee = CE->getCalleeDecl();
-    const Function *Func = P.getFunction(dyn_cast<FunctionDecl>(Callee));
+    const Function *Func = getFunction(dyn_cast<FunctionDecl>(Callee));
+
----------------
Any reason not to use `cast` here instead, given that `getFunction()` expects a nonnull pointer anyway?


================
Comment at: clang/test/AST/Interp/records.cpp:106
+
+constexpr C RVOAndParams(const C *c) {
+  return C();
----------------
We're missing a fair amount of test coverage here in terms of calling member functions. Can you add some tests for that, as well as a test where the this pointer is invalid and causes UB? e.g.,
```
struct S {
  constexpr void member() {}
};

constexpr void func(S *s) {
  s->member(); // Should not be a constant expression
}

int main() {
  func(nullptr);
}
```


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

https://reviews.llvm.org/D134699