aaron.ballman added inline comments.
================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:749-751
+ const Descriptor::MetadataSize MDSize{sizeof(InlineDescriptor)};
+ Descriptor *D =
+ P.createDescriptor(Src, Ty, MDSize, IsConst, Src.is<const Expr *>());
----------------
tbaeder wrote:
> erichkeane wrote:
> > aaron.ballman wrote:
> > > Double checking that `Src.is<const Expr *>()` is correct here. That's
> > > being passed as the `IsTemporary` argument. I presume the logic is that
> > > if the decl type is an expression then it has to be a temporary, but
> > > that's not entirely true. Consider C where a compound literal is *not* a
> > > temporary but actually creates an lvalue: `(int){12}`. Will that be an
> > > issue?
> > See suggested comment below in InterpBlock.
> That interpretation is correct and I don't know what to do about compound
> literals yet.
My intuition is that `IsTemporary` should be set from
`Expr::isTemporaryObject()` (or isa `MaterializeTemporaryExpr` maybe).
At the very least, you can add a .c test file with a compound literal in a
_Static_assert and a FIXME comment (in the test and here in the code).
================
Comment at: clang/lib/AST/Interp/Descriptor.h:74
+ };
+ static constexpr MetadataSize NoMetadata = MetadataSize{0};
+
----------------
tbaeder wrote:
> tbaeder wrote:
> > shafik wrote:
> > > erichkeane wrote:
> > > > add a line:
> > > > `static constexpr MetadataSize InlineDescriptor =
> > > > MetadataSize{sizeof(InlineDescriptor)};`
> > > > and you can use this instead of depending on 'sizeof' all over the
> > > > place.
> > > It feels weird to call this `NoMetadata` but we will pass this as an
> > > argument to a function with a parameter of `MetaDataSize`. So I am
> > > expecting a size but I am getting no meta data instead and it looks like
> > > a mistake.
> > >
> > > Maybe a better name would be `ZeroMetaDataSize`?
> > I kinda get what your point is, but I don't think this is confusing.
> I'm a bit concerned about the naming here; `InlineDescriptor` is already a
> type name, but calling it `MetadataSizeInlineDesc` is pretty long and callers
> already have to prepend a `Descriptor::` to that. :/
Rather than using a sentinel value, should we be using `Optional`?
================
Comment at: clang/lib/AST/Interp/Interp.h:950
return false;
if (auto *FD = Ptr.getField()) {
Ptr.deref<T>() = Value.truncate(FD->getBitWidthValue(S.getCtx()));
----------------
tbaeder wrote:
> aaron.ballman wrote:
> > Do we need to `initialize()` here as well?
> I think so, but bitfields are currently completely untested.
That should be rectified. :-) It seems like that's reasonable to do as part of
this patch given it's mostly just two changes here and a few test cases?
================
Comment at: clang/lib/AST/Interp/InterpBlock.h:77-86
+ char *data() {
+ // Data might contain metadata as well.
+ size_t DataOffset = Desc->getMetadataSize();
+ return reinterpret_cast<char *>(reinterpret_cast<char *>(this) +
+ sizeof(Block) + DataOffset);
+ }
+
----------------
tbaeder wrote:
> aaron.ballman wrote:
> > Should we have `const`-correct overloads to provide read-only access?
> IIRC I added that at least once in a different patch but I abandoned that and
> haven't had a use for a const overload since.
My worry is that retrofitting const correctness will eventually become too much
of a chore and so we'll be adding yet another const-incorrect interface to the
compiler. Seeing as how this is being reworked from scratch, I think we should
use the opportunity to get better const-correctness, but maybe that ship has
sailed?
================
Comment at: clang/lib/AST/Interp/InterpBlock.h:97
void invokeCtor() {
- std::memset(data(), 0, getSize());
+ std::memset(rawData(), 0, Desc->getAllocSize());
if (Desc->CtorFn)
----------------
tbaeder wrote:
> aaron.ballman wrote:
> > Why do we want to overwrite the metadata here?
> This is only called after creating an new block, so nothing is being
> overwritten, the metadata hasn't been filled-in yet.
Sounds like a good reason not to memset over that block then; it's useless work
that will be thrown away anyway (I worry we may come to rely on this zero init
accidentally)?
================
Comment at: clang/lib/AST/Interp/Program.cpp:334
Descriptor *ElemDesc =
- createDescriptor(D, ElemTy.getTypePtr(), IsConst, IsTemporary);
+ createDescriptor(D, ElemTy.getTypePtr(), Descriptor::NoMetadata,
+ IsConst, IsTemporary);
----------------
tbaeder wrote:
> aaron.ballman wrote:
> > Why does this one get no metadata while the rest of the descriptors created
> > do?
> This is the descriptor for the array elements. The metadata passed to this
> function is only being used for the outermost descriptor, i.e. we use it a
> few lines below for the actual array but we don't use it for the array
> elements.
Don't we need the metadata to track whether the array elements have been
initialized? e.g., it's not the array object that gets initialized, it's the
elements that do, and we'd want to catch things like:
```
consteval int func() {
int a[10];
return a[2];
}
```
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135750/new/
https://reviews.llvm.org/D135750
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
