[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:1127
         return false;
-      if (!this->emitInitGlobal(*T, *I, VD))
+    }
+  } else {
----------------
tbaeder wrote:
> aaron.ballman wrote:
> > and if we have no `GlobalIndex`?
> > 
> > Should this instead be:
> > ```
> > if (auto GlobalIndex = P.getGlobal(VD); !GlobalIndex || !this->emitGetPtrGlobal(*GlobalIndex, VD))
> >   return false;
> > ```
> > 
> I mean, that shouldn't happen because the `visitVarDecl` call before would've returned `false` in that case.
The use of an `if` makes it seem like it could fail. Perhaps we should drop the `if` and use an `assert`?


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:1161
+      if (VarT) {
+        if (!this->visit(Init))
           return false;
----------------
tbaeder wrote:
> aaron.ballman wrote:
> > What if `Init` is `nullptr`?
> Global variables (or local constexpr ones) must have an initializer, no?
Hmm, I was thinking of:
```
struct S {};
constexpr S s;
```
but now that I think on it, that still has a non-null init expression, so I think it's safe for us to assert `Init` is nonnull.


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.h:262
+  /// given VarDecl.
+  bool ShouldBeGloballyIndexed(const VarDecl *VD) const {
+    return !VD->hasLocalStorage() || VD->isConstexpr();
----------------



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

https://reviews.llvm.org/D136815