[PATCH] D134057: [clang][Interp] Start implementing record types

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:642
+
+    for (const auto *Init : Ctor->inits()) {
+      const FieldDecl *Member = Init->getMember();
----------------
Do we have to do anything special if the ctor is an inheriting ctor?


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:687
+    return true;
+  } else if (const CallExpr *CE = dyn_cast<CallExpr>(Initializer)) {
+    const Decl *Callee = CE->getCalleeDecl();
----------------
`CXXMemberCallExpr` as well?


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:689
+    const Decl *Callee = CE->getCalleeDecl();
+    const Function *Func = P.getFunction(dyn_cast<FunctionDecl>(Callee));
+
----------------
What if this comes back as `nullptr` (does `getFunction()` handle that gracefully)?


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:316
+  // Base above gives us a pointer on the stack.
+  const auto *FD = dyn_cast<FieldDecl>(Member);
+  assert(FD);
----------------
shafik wrote:
> tbaeder wrote:
> > erichkeane wrote:
> > > I THINK Member is a ValueDecl because it could be a member function, right?  So forcing it to be a FieldDecl here is likely not valid.  Perhaps in the 'non-FieldDecl' case we could jsut return false for now?
> > > 
> > > ALSO, don't do a dyn_cast followed by an assert, `cast` will do the assert for you.
> > Right, I was just trying to limit the code to the subset I've implemented for now. I can try to make it more defensive.
> Also I believe this can also be an `IndirectFieldDecl` (anonymous union members) or a `ValueDecl` for static data members. 
> 
> Maybe outline what is left to fill in a comment?
There are quite a few kinds of declarations, but many of them don't matter or only matter in special circumstances. Like `StaticAssertDecl` shouldn't be something we can visit here, but technically `TypedefDecl` can because of VLA evaluations. `MSPropertyDecl` is one that could possibly matter as well.


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

https://reviews.llvm.org/D134057