tbaeder marked 10 inline comments as done.
tbaeder 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; }
----------------
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.
================
Comment at: clang/lib/AST/Interp/Descriptor.h:74
+ };
+ static constexpr MetadataSize NoMetadata = MetadataSize{0};
+
----------------
aaron.ballman wrote:
> aaron.ballman wrote:
> > 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`?
> I'm still wondering if `Optional` is cleaner than `NoMetadata`...
New version uses `Optional<InterpSize>`.
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
