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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I don't have enough knowledge about how this works to do a better review, but I have a couple of suggestions.



================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:749
                                                           bool IsExtended) {
-  Descriptor *D = P.createDescriptor(Src, Ty, IsConst, Src.is<const Expr *>());
+  const Descriptor::MetadataSize MDSize{sizeof(InlineDescriptor)};
+  Descriptor *D =
----------------
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.


================
Comment at: clang/lib/AST/Interp/Descriptor.cpp:207
     : Source(D), ElemSize(primSize(Type)), Size(ElemSize * NumElems),
-      AllocSize(align(Size) + sizeof(InitMap *)), IsConst(IsConst),
-      IsMutable(IsMutable), IsTemporary(IsTemporary), IsArray(true),
-      CtorFn(getCtorArrayPrim(Type)), DtorFn(getDtorArrayPrim(Type)),
-      MoveFn(getMoveArrayPrim(Type)) {
+      MDSize(MD.NumBytes), AllocSize(align(Size) + sizeof(InitMap *) + MDSize),
+      IsConst(IsConst), IsMutable(IsMutable), IsTemporary(IsTemporary),
----------------
Theres enough math on stuff like AllocSize/etc that we should probably:

1- do SOMETHING to make sure we're not overflowing
2- Have assertions to make sure we got the sizes looking reasonable.

Additionally, is there any ability to refactor out the common subset of initializers into a constructor and delegate to them instead of all these really fragile looking changes?


================
Comment at: clang/lib/AST/Interp/Descriptor.h:74
+  };
+  static constexpr MetadataSize NoMetadata = MetadataSize{0};
+
----------------
add a line: 
`static constexpr MetadataSize InlineDescriptor = MetadataSize{sizeof(InlineDescriptor)};` 
and you can use this instead of depending on 'sizeof' all over the place.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135750/new/

https://reviews.llvm.org/D135750