[PATCH] D136815: [clang][Interp] Unify visiting variable declarations

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:803
+  // Make sure we don't accidentally register the same decl twice.
+  if (auto *VD = dyn_cast_if_present<ValueDecl>(Src.dyn_cast<const Decl *>())) {
+    assert(!P.getGlobal(VD));
----------------



================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:822
+  // Make sure we don't accidentally register the same decl twice.
+  if (auto *VD = dyn_cast_if_present<ValueDecl>(Src.dyn_cast<const Decl *>())) {
+    assert(!P.getGlobal(VD));
----------------



================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:1127
         return false;
-      if (!this->emitInitGlobal(*T, *I, VD))
+    }
+  } else {
----------------
and if we have no `GlobalIndex`?

Should this instead be:
```
if (auto GlobalIndex = P.getGlobal(VD); !GlobalIndex || !this->emitGetPtrGlobal(*GlobalIndex, VD))
  return false;
```



================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:1129-1131
+    if (auto It = Locals.find(VD); It != Locals.end()) {
+      if (!this->emitGetPtrLocal(It->second.Offset, VD))
         return false;
----------------
Similar question here as above -- what if `It == Locals.end()`?


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:1161
+      if (VarT) {
+        if (!this->visit(Init))
           return false;
----------------
What if `Init` is `nullptr`?


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:1165
       }
+      return this->visitGlobalInitializer(Init, *GlobalIndex);
+    }
----------------
Same here?


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.h:96
   bool visitDecl(const VarDecl *VD) override;
+  bool visitVarDecl(const VarDecl *VD);
 
----------------
I was a bit surprised to not see `override` here -- I wonder if we should have some visual separation between the overrides and the non-overrides given the similarity in naming?


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.h:282
+  bool isGlobalDecl(const VarDecl *VD) const {
+    return !VD->hasLocalStorage() || VD->isConstexpr();
+  }
----------------
tbaeder wrote:
> shafik wrote:
> > tbaeder wrote:
> > > shafik wrote:
> > > > tbaeder wrote:
> > > > > shafik wrote:
> > > > > > Why not `hasGlobalStorage()`?
> > > > > > 
> > > > > > Also what is the logic behind `isConstexpr()` check?
> > > > > Didn't know `isGlobalStorage()` existed ;)
> > > > > 
> > > > > Constexpr local variables can be handled like global ones, can't they? That was the logic behind it, nothing else. We can save ourselves the hassle of local variables in that case.
> > > > I think I am missing a level of logic here. I don't think of constant expressions as needing storage nor do I think of them as global variables.
> > > > 
> > > > So can you take a step back and explain how this fits in the bigger picture?
> > > They don't necessarily need storage in the final executable but we create global/local variables for them for bookkeeping, e.g. we need to be able to take their address, track the value, etc.
> > Ok, will this work for recursive `constexpr` functions with local variables?
> local `constexpr` variables still have to be initialized and are immutable, so that doesn't make a difference for recursive functions since none of the recursive calls can change the variable value. I did a small local test but since the variable can't be modified, it's not very interesting.
I find the interface to be confusing. If I want to know "is this a global decl", I am not going to expect a local constexpr variable to go "oh yeah, I totally am!". Perhaps this should be renamed to something more about the semantics, like `shouldBeGloballyIndexed()` or something along those lines?


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

https://reviews.llvm.org/D136815