[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.cpp:803
+  // Make sure we don't accidentally register the same decl twice.
+  if (const Decl *VD = Src.dyn_cast<const Decl *>(); VD && isa<ValueDecl>(VD)) {
+    assert(!P.getGlobal(cast<ValueDecl>(VD)));
----------------
shafik wrote:
> Nitpick, I find using `VD` a bit confusing since we don't know if it is a `ValueDecl` until after it is initialized. 
Replaced this with the same version we use a few lines below anyway.


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


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

https://reviews.llvm.org/D136815