[PATCH] D135750: [clang][Interp] Track initialization state of local variables
Shafik Yaghmour via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Dec 20 11:23:50 PST 2022
shafik added a comment.
This looks good to me but I see at least one concern that Aaron had that he did not get back on, so I will wait for him to approve.
================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:835
IsTemporary = true;
Ty = E->getType();
}
----------------
tbaeder wrote:
> shafik wrote:
> > Do we really want to the type of the expression? If we have a `ValueDecl` don't we want that type? I think they should be the same, do you have any examples where the expression is the type we want? I am looking at the AST from ` int x = 1+1L;`
> >
> > https://godbolt.org/z/q3Tr7Kxoq
> I don't have a counter example but I didn't write that line either. The Expr case is for temporary values created for AST expressions, not sure what other type to use.
I think it should be an `else if` we either have a `Decl` or an `Expr` right?
================
Comment at: clang/lib/AST/Interp/ByteCodeStmtGen.cpp:408
// Compile the initializer in its own scope.
- {
+ if (Init) {
ExprScope<Emitter> Scope(this);
----------------
So by moving the check for `Init` forward, we will still allocate but we may not initialize.
================
Comment at: clang/lib/AST/Interp/Descriptor.h:73
+ /// Flag indicating if the field is mutable (if in a record).
+ unsigned IsMutable : 1;
+
----------------
aaron.ballman wrote:
> tbaeder wrote:
> > shafik wrote:
> > > Maybe `IsFieldMutable` b/c we call `CreateDescriptor` it is a little confusing why we have `IsConst` and `IsMutable`
> > Agreed, but this code was just moved up in this patch, the field was introduced earlier.
> We can do a renaming pass once this lands, but FWIW, I'm also fine clarifying the names of things as we refactor code.
I am happy fixing this separately, please leave a `TODO`
================
Comment at: clang/lib/AST/Interp/Interp.h:918
return false;
+ if (!Ptr.isRoot())
+ Ptr.initialize();
----------------
Can you explain what is going on here? Why do we need to initialize if it is not root?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135750/new/
https://reviews.llvm.org/D135750
More information about the cfe-commits
mailing list