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

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik 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)));
----------------
Nitpick, I find using `VD` a bit confusing since we don't know if it is a `ValueDecl` until after it is initialized. 


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:1154
+
+    if (!GlobalIndex)
+      return this->bail(VD);
----------------
I wonder if the code in both branches might be better factored out into their own functions and perhaps move the ` DeclScope<Emitter> LocalScope(this, VD);` out.


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.h:282
+  bool isGlobalDecl(const VarDecl *VD) const {
+    return !VD->hasLocalStorage() || VD->isConstexpr();
+  }
----------------
Why not `hasGlobalStorage()`?

Also what is the logic behind `isConstexpr()` check?


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

https://reviews.llvm.org/D136815