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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits