[PATCH] D135750: [clang][Interp] Track initialization state of local variables

[Timm Bäder via Phabricator via cfe-commits]

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