tbaeder marked 4 inline comments as done.
tbaeder 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 *>());
----------------
aaron.ballman wrote:
> 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).
>
The `Src.is<>` is not a change introduced by this patch though.
As for a test, it'll just be rejected because we don't implement visiting
`CompoundLiteral`s at all right now.
================
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()));
----------------
aaron.ballman wrote:
> 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?
I can add the `initialize()` calls but I don't think adding bitfield test cases
makes sense for this patch (pretty sure they will fail in unexpected ways
anyway).
================
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);
+ }
+
----------------
aaron.ballman wrote:
> 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?
I don't think the ship has sailed at all. Is your argument that the function
should be `const` because it can be? It's slightly problematic because they end
up returning `const char *` as well. I can add the two overloads without
problem, just not sure if they end up being used at all right now.
================
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);
----------------
aaron.ballman wrote:
> 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];
> }
> ```
That's what the `InitMap` is for: it's just a bitmap storing one bit per
element of a primitive array for its initialization state.
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
