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