[PATCH] D41102: Setup clang-doc frontend framework

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Could some other people please review this differential, too?
I'm sure i have missed things.

---

Some more nitpicking.

For this differential as standalone, i'we mostly run out of things to nitpick.
Some things can probably be done better (the blockid/recordid stuff could probably be nicer if tablegen-ed, but that is for later).

I'll try to look at the next differential, and at them combined.



================
Comment at: clang-doc/BitcodeWriter.cpp:120
+        BlockIdNameMap[Init.first] = Init.second;
+        assert((BlockIdNameMap[Init.first].size() + 1) <=
+               BitCodeConstants::RecordSize);
----------------
We don't actually push these strings to the `Record` (but instead output them directly), so this assertion is not really meaningful, i think?


================
Comment at: clang-doc/BitcodeWriter.h:21
+#include "clang/AST/AST.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/Bitcode/BitstreamWriter.h"
----------------
+DenseMap


================
Comment at: clang-doc/BitcodeWriter.h:21
+#include "clang/AST/AST.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/Bitcode/BitstreamWriter.h"
----------------
lebedev.ri wrote:
> +DenseMap
+StringRef


================
Comment at: clang-doc/BitcodeWriter.h:197
+        : Stream(Stream_) {
+      Stream.EnterSubblock(ID, BitCodeConstants::SubblockIDSize);
+    }
----------------
Humm, you could avoid this constant, and conserve a few bits, if you move the init-list out of `emitBlockInfoBlock()` to somewhere e.g. after the `enum RecordId`, and then since the `BlockId ID` is already passed, you could compute it on-the-fly the same way the `BitCodeConstants::SubblockIDSize` is asserted in `emitBlockInfo*()`.
Not sure if it's worth doing though. Maybe just add it as a `NOTE` here.


================
Comment at: clang-doc/BitcodeWriter.h:249
+///
+/// \param WriteBlockInfo
+/// For serializing a single info (as in the mapper
----------------
Stale comment


================
Comment at: clang-doc/Representation.h:60
+  InfoType RefType = InfoType::IT_default;
+  Info *Ref;
+};
----------------
`Info *Ref;` isn't used anywhere


================
Comment at: clang-doc/Representation.h:117
+  bool IsDefinition = false;
+  Location DefLoc;
+  llvm::SmallVector<Location, 2> Loc;
----------------
`llvm::Optional<Location> DefLoc;`  ?


https://reviews.llvm.org/D41102