[PATCH] D63371: Rewrite ConstStructBuilder with a mechanism that can cope with splitting and updating constants.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: lib/CodeGen/CGExprConstant.cpp:73
+/// Incremental builder for an llvm::Constant* holding a structure constant.
+class ConstantBuilder : private ConstantBuilderUtils {
+  llvm::SmallVector<llvm::Constant*, 32> Elems;
----------------
This seems like a very generic name for this type.


================
Comment at: lib/CodeGen/CGExprConstant.cpp:75
+  llvm::SmallVector<llvm::Constant*, 32> Elems;
+  llvm::SmallVector<CharUnits, 32> Offsets;
+  CharUnits Size = CharUnits::Zero();
----------------
Are there invariants about these?  I assume they're parallel arrays; are they kept sorted?


================
Comment at: lib/CodeGen/CGExprConstant.cpp:76
+  llvm::SmallVector<CharUnits, 32> Offsets;
+  CharUnits Size = CharUnits::Zero();
+
----------------
This is one past the last byte that's been covered by an actual `Constant*` value, or does it include unoccupied padding, or does it exclude even occupied padding?


================
Comment at: lib/CodeGen/CGExprConstant.cpp:98
+    Offsets.reserve(N);
+  }
+
----------------
Might be worth clarifying what `N` is here.


================
Comment at: lib/CodeGen/CGExprConstant.cpp:190
+bool ConstantBuilder::addBits(llvm::APInt Bits, uint64_t OffsetInBits,
+                              bool AllowOverwrite) {
+  const ASTContext &Context = CGM.getContext();
----------------
`AllowOversized` (which you used in the interface) seems like a better name.


================
Comment at: lib/CodeGen/CGExprConstant.cpp:196
+  // current char.
+  unsigned CharOffset = OffsetInBits % CharWidth;
+
----------------
`OffsetWithinChar`?


================
Comment at: lib/CodeGen/CGExprConstant.cpp:201
+  for (CharUnits Char = Context.toCharUnitsFromBits(OffsetInBits);
+       /**/; ++Char) {
+    // Number of bits we want to fill in this byte.
----------------
`OffsetInChars`?


================
Comment at: lib/CodeGen/CGExprConstant.cpp:237
+      if (!LastElemToUpdate)
+        return false;
+      assert(*LastElemToUpdate - *FirstElemToUpdate < 2 &&
----------------
Especially in the context of the comment above, I think it would be good to clarify that both of these are hard "we can't emit this constant" bail-outs.


================
Comment at: lib/CodeGen/CGExprConstant.cpp:258
+          return false;
+        assert(CI->getBitWidth() == CharWidth && "splitAt failed");
+        assert((!(CI->getValue() & UpdateMask) || AllowOverwrite) &&
----------------
Oh, because we're splitting before and after a single-`CharUnits` range?  That seems worthy of a somewhat clearer explanation in the code.

I guess we could have a non-`ConstantInt` single-byte value.  Unlikely but not impossible. :)


================
Comment at: lib/CodeGen/CGExprConstant.cpp:375
+
+  // FIXME: We could theoretically split a ConstantInt if the need ever arose.
+
----------------
Does this not come up all the time with bit-fields?  I guess we emit them in single-`char` chunks, so it wouldn't.  Probably worth a comment.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63371