aaron.ballman added inline comments.
================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:803
+ // Make sure we don't accidentally register the same decl twice.
+ if (auto *VD = dyn_cast_if_present<ValueDecl>(Src.dyn_cast<const Decl *>()))
{
+ assert(!P.getGlobal(VD));
----------------
================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:822
+ // Make sure we don't accidentally register the same decl twice.
+ if (auto *VD = dyn_cast_if_present<ValueDecl>(Src.dyn_cast<const Decl *>()))
{
+ assert(!P.getGlobal(VD));
----------------
================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:1127
return false;
- if (!this->emitInitGlobal(*T, *I, VD))
+ }
+ } else {
----------------
and if we have no `GlobalIndex`?
Should this instead be:
```
if (auto GlobalIndex = P.getGlobal(VD); !GlobalIndex ||
!this->emitGetPtrGlobal(*GlobalIndex, VD))
return false;
```
================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:1129-1131
+ if (auto It = Locals.find(VD); It != Locals.end()) {
+ if (!this->emitGetPtrLocal(It->second.Offset, VD))
return false;
----------------
Similar question here as above -- what if `It == Locals.end()`?
================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:1161
+ if (VarT) {
+ if (!this->visit(Init))
return false;
----------------
What if `Init` is `nullptr`?
================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:1165
}
+ return this->visitGlobalInitializer(Init, *GlobalIndex);
+ }
----------------
Same here?
================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.h:96
bool visitDecl(const VarDecl *VD) override;
+ bool visitVarDecl(const VarDecl *VD);
----------------
I was a bit surprised to not see `override` here -- I wonder if we should have
some visual separation between the overrides and the non-overrides given the
similarity in naming?
================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.h:282
+ bool isGlobalDecl(const VarDecl *VD) const {
+ return !VD->hasLocalStorage() || VD->isConstexpr();
+ }
----------------
tbaeder wrote:
> 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.
I find the interface to be confusing. If I want to know "is this a global
decl", I am not going to expect a local constexpr variable to go "oh yeah, I
totally am!". Perhaps this should be renamed to something more about the
semantics, like `shouldBeGloballyIndexed()` or something along those lines?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136815/new/
https://reviews.llvm.org/D136815
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
