[PATCH] D41102: Setup clang-doc frontend framework

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Tried fixing `tooling::FrontendActionFactory::create()` in https://reviews.llvm.org/D43779/https://reviews.llvm.org/D43780, but had to revert due to gcc4.8 issues :/

Thank you for working on this, some more review notes.

In https://reviews.llvm.org/D41102#1020107, @juliehockett wrote:

> In https://reviews.llvm.org/D41102#1017918, @lebedev.ri wrote:
>
> > 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`?
>
>
> Yes -- it will fail an assertion:
>  `Assertion 'V == Op.getLiteralValue() && "Invalid abbrev for record!"' failed.`


Ok, great.
And it will also complain if you try to output a block within block?



================
Comment at: clang-doc/BitcodeWriter.cpp:184
+          return RecordIdNameMap;
+        }  // namespace doc
+();
----------------
That comment seems wrong.
**If** the namespace is indeed supposed to be closed, it should happen after the lambda is called, i.e.
```
          assert(RecordIdNameMap.size() == RecordIdCount);
          return RecordIdNameMap;
        }();
}  // namespace doc

// AbbreviationMap


================
Comment at: clang-doc/BitcodeWriter.cpp:265
+  if (!prepRecordData(ID, !OmitFilenames)) return;
+  // FIXME: Assert that the line number is of the appropriate size.
+  Record.push_back(Loc.LineNumber);
----------------
I think it is as simple as
```
assert(Loc.LineNumber < (1U << BitCodeConstants::LineNumberSize));
```
?


================
Comment at: clang-doc/BitcodeWriter.cpp:367
+    BlockId BID, const std::initializer_list<RecordId> &RIDs) {
+  emitBlockID(BID);
+  for (RecordId RID : RIDs) {
----------------
So i guess this should be:
```
void ClangDocBitcodeWriter::emitBlockInfo(
    BlockId BID, const std::initializer_list<RecordId> &RIDs) {
  assert(RIDs.size() < (1U << BitCodeConstants::SubblockIDSize), "Too many records in a block!");
  emitBlockID(BID);
...
```
?


================
Comment at: clang-doc/BitcodeWriter.cpp:191
+  Record.clear();
+  for (const char C : BlockIdNameMap[ID]) Record.push_back(C);
+  Stream.EmitRecord(llvm::bitc::BLOCKINFO_CODE_BLOCKNAME, Record);
----------------
juliehockett wrote:
> lebedev.ri wrote:
> > Why do we have this indirection?
> > Is there a need to first to (unefficiently?) copy to `Record`, and then emit from there?
> > Wouldn't this work just as well?
> > ```
> > Record.clear();
> > Stream.EmitRecord(llvm::bitc::BLOCKINFO_CODE_BLOCKNAME, BlockIdNameMap[ID]);
> > ```
> No, since `BlockIdNameMap[ID]` returns a `StringRef`, which can be manipulated into an `std::string` or a `const char*`, but the `Stream` wants an `unsigned char`. So, the copying is to satisfy that. Unless there's a better way to convert a `StringRef` into an array of `unsigned char`?
Aha, i see, did not think of that.
But there is a `bytes()` function in `StringRef`, which returns `iterator_range<const unsigned char *>`. 
Would it help? http://llvm.org/doxygen/classllvm_1_1StringRef.html#a5e8f22c3553e341404b445430a3b075b


================
Comment at: clang-doc/BitcodeWriter.cpp:240
+  if (!prepRecordData(ID, Val)) return;
+  assert(Val < (1U << BitCodeConstants::IntSize));
+  Record.push_back(Val);
----------------
juliehockett wrote:
> lebedev.ri wrote:
> > 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)
> That assertion fails :/ I could do something like `static_cast<int64_t>(Val) == Val` but that would require a) IntSize being a power of 2 b) updating the assert anytime IntSize is updated, and 3) still throws a warning about comparing a signed to an unsigned int...
I see. Let's not have this assertion for now, just a `FIXME`.


================
Comment at: clang-doc/BitcodeWriter.h:53
+  BI_LAST = BI_COMMENT_BLOCK_ID
+};
+
----------------
juliehockett wrote:
> lebedev.ri wrote:
> > 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.
> No...it's the (max) number of the abbrevs relevant to the block itself, which is to say some subset of the RecordIds for any given block (e.g. for a `BI_COMMENT_BLOCK`,  the number of abbrevs would be 12 and so on the abbrev width would be 4). 
> 
> To assert for it we could put block start/end markers on the RecordIds and then use that to calculate the bitwidth, if you think the assertion should be there.
Aha, i see, so that should go into `ClangDocBitcodeWriter::emitBlockInfoBlock()`, since that already has that info.
(On a related node, it feels like this all should be somehow tablegen-generated, but that is for some later, post-commit cleanup.)


https://reviews.llvm.org/D41102