[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:
> > 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`?
> What do you think about returning a `tuple(valid, offset)` from `visitVarDecl`? Although that might make the call in `ByteCodeStmtGen.cpp` a little awkward.
Hmmmm, I don't think I'd like to see a tuple, but seeing an `llvm::ErrorOr<>` or a `std::optional<>` might be reasonable. The downside is that the `visit*` functions then won't have a uniform return type (can't do `ErrorOr<void>` that I'm aware of)


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

https://reviews.llvm.org/D136815