lebedev.ri added a comment. Could you please add a bit more tests? In particular, i'd like to see how blocks-in-blocks work. I.e. class-in-class, class-in-function, ...
Is there some (internal to `BitstreamWriter`) logic that would 'assert()' if trying to output some recordid which is, according to the `BLOCKINFO_BLOCK`, should not be there? E.g. outputting `VERSION` in `BI_COMMENT_BLOCK_ID`? ================ Comment at: clang-doc/BitcodeWriter.cpp:30 + +static void IntAbbrev(std::shared_ptr<llvm::BitCodeAbbrev> &Abbrev) { + Abbrev->Add(llvm::BitCodeAbbrevOp(llvm::BitCodeAbbrevOp::Fixed, ---------------- Ok, these three functions still look off, how about this? ``` // Yes, not by reference, https://godbolt.org/g/T52Vcj static void AbbrevGen(std::shared_ptr<llvm::BitCodeAbbrev> &Abbrev, const std::initializer_list<llvm::BitCodeAbbrevOp> Ops) { for(const auto &Op : Ops) Abbrev->Add(Op); } static void IntAbbrev(std::shared_ptr<llvm::BitCodeAbbrev> &Abbrev) { AbbrevGen(Abbrev, { // 0. Fixed-size integer {llvm::BitCodeAbbrevOp::Fixed, BitCodeConstants::IntSize}}); } static void StringAbbrev(std::shared_ptr<llvm::BitCodeAbbrev> &Abbrev) { AbbrevGen(Abbrev, { // 0. Fixed-size integer (length of the following string) {llvm::BitCodeAbbrevOp::Fixed, BitCodeConstants::StringLengthSize}, // 1. The string blob {llvm::BitCodeAbbrevOp::Blob}}); } // Assumes that the file will not have more than 65535 lines. static void LocationAbbrev(std::shared_ptr<llvm::BitCodeAbbrev> &Abbrev) { AbbrevGen(Abbrev, { // 0. Fixed-size integer (line number) {llvm::BitCodeAbbrevOp::Fixed, BitCodeConstants::LineNumberSize}, // 1. Fixed-size integer (length of the following string (filename)) {llvm::BitCodeAbbrevOp::Fixed, BitCodeConstants::StringLengthSize}, // 2. the string blob {llvm::BitCodeAbbrevOp::Blob}}); } ``` Though i bet clang-format will mess-up the formatting again :/ ================ Comment at: clang-doc/BitcodeWriter.cpp:108 + {COMMENT_CLOSENAME, {"CloseName", &StringAbbrev}}, + {COMMENT_SELFCLOSING, {"SelfClosing", &IntAbbrev}}, + {COMMENT_EXPLICIT, {"Explicit", &IntAbbrev}}, ---------------- Some of these `IntAbbrev`'s are actually `bool`s. Would it make sense to already think about being bitcode-size-conservative and introduce `BoolAbbrev` from the get go? ``` static void BoolAbbrev(std::shared_ptr<llvm::BitCodeAbbrev> &Abbrev) { AbbrevGen(Abbrev, { // 0. Fixed-size boolean {llvm::BitCodeAbbrevOp::Fixed, BitCodeConstants::BoolSize}}); } ``` where `BitCodeConstants::BoolSize` = `1U` ? Or is there some internal padding that would make that pointless? ================ Comment at: clang-doc/BitcodeWriter.cpp:156 + unsigned AbbrevID) { + assert(RecordIdNameMap[RID] && "Unknown Abbreviation"); + assert(Abbrevs.find(RID) == Abbrevs.end() && "Abbreviation already added."); ---------------- Uh, oh, i'm sorry, all(?) these `"Unknown Abbreviation"` are likely copypaste gone wrong. I'm not sure why i wrote that comment. `"Unknown RecordId"` might make more sense? ================ Comment at: clang-doc/BitcodeWriter.cpp:240 + if (!prepRecordData(ID, Val)) return; + assert(Val < (1U << BitCodeConstants::IntSize)); + Record.push_back(Val); ---------------- Ok, now that i think about it, it can't be that easy. Maybe ``` FIXME: assumes 8 bits per byte assert(llvm::APInt(8U*sizeof(Val), Val, /*isSigned=*/true).getBitWidth() <= BitCodeConstants::IntSize)); ``` Not sure whether `getBitWidth()` is really the right function to ask though. (Not sure how this all works for negative numbers) ================ Comment at: clang-doc/BitcodeWriter.h:53 + BI_LAST = BI_COMMENT_BLOCK_ID +}; + ---------------- juliehockett wrote: > lebedev.ri wrote: > > So what *exactly* does `BitCodeConstants::SubblockIDSize` mean? > > ``` > > static_assert(BI_LAST < (1U << BitCodeConstants::SubblockIDSize), "Too many > > block id's!"); > > ``` > > ? > It's the current abbrev id width for the block (described [[ > https://llvm.org/docs/BitCodeFormat.html#enter-subblock-encoding | here ]]), > so it's the max id width for the block's abbrevs. So in other words that `static_assert()` is doing the right thing? Add it after the `enum BlockId{}` then please, will both document things, and ensure that things remain in a sane state. ================ Comment at: clang-doc/BitcodeWriter.h:172 + AbbreviationMap() : Abbrevs(RecordIdCount) {} + void add(RecordId RID, unsigned AbbrevID); + unsigned get(RecordId RID) const; ---------------- Newline after constructor ================ Comment at: clang-doc/BitcodeWriter.h:216 + + // Emission of different abbreviation types + void emitAbbrev(RecordId ID, BlockId Block); ---------------- `// Emission of appropriate abbreviation type` https://reviews.llvm.org/D41102 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits