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

[Aaron Ballman via Phabricator via cfe-commits]

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