aaron.ballman added inline comments.
================
Comment at: clang/lib/AST/Interp/Descriptor.h:141-145
unsigned getAllocSize() const { return AllocSize; }
/// returns the size of an element when the structure is viewed as an array.
unsigned getElemSize() const { return ElemSize; }
+ /// Returns the size of the metadata.
+ unsigned getMetadataSize() const { return MDSize; }
----------------
tbaeder wrote:
> aaron.ballman wrote:
> > There's some interface awkwardness here where these values are of type
> > `InterpSize` but the interface returns `unsigned`
> Yes. `InterpSize` is also only really used in `Descriptor.h`. No idea why.
Yeah, this should be cleaned up (either here or in an NFC follow-up soon after
this lands).
================
Comment at: clang/lib/AST/Interp/Descriptor.h:73
+ /// Flag indicating if the field is mutable (if in a record).
+ unsigned IsMutable : 1;
+
----------------
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.
================
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:
> > 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)?
> FWIW I looked into this and I think zeroing everything is what we want, so
> the initmap is null initially
Hmmm, would it make more sense to have the init map setting that itself (in
`allocate()`) rather than relying on `invokeCtor()` to do it?
================
Comment at: clang/lib/AST/Interp/Pointer.h:63-64
private:
static constexpr unsigned PastEndMark = (unsigned)-1;
static constexpr unsigned RootPtrMark = (unsigned)-1;
----------------
tbaeder wrote:
> shafik wrote:
> > or `-1u`
> Alright, but those were not introduced in this patch.
Preference for `~0u` so the types all match up nicely, fine to do in a
post-commit NFC change.
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
