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

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added inline comments.


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.h:282
+  bool isGlobalDecl(const VarDecl *VD) const {
+    return !VD->hasLocalStorage() || VD->isConstexpr();
+  }
----------------
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.


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

https://reviews.llvm.org/D136815