[PATCH] D41102: Setup clang-doc frontend framework

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Thank you for working on this!
Some more thoughts.



================
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);
----------------
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]);
```


================
Comment at: clang-doc/BitcodeWriter.cpp:196
+/// \brief Emits a record name to the BLOCKINFO block.
+void ClangDocBitcodeWriter::emitRecordID(RecordId ID) {
+  assert(RecordIdNameMap[ID] && "Unknown Abbreviation");
----------------
Hmm, so i've been staring at this and http://llvm.org/doxygen/classllvm_1_1BitstreamWriter.html and i must say i'm not fond of this indirection.

What i don't understand is, in previous function, we don't store `BlockId`, why do we want to store `RecordId`?
Aren't they both unstable, and are implementation detail?
Do we want to store it (`RecordId`)? If yes, please explain it as a new comment in code.

If no, i guess this would work too?
```
assert(RecordIdNameMap[ID] && "Unknown Abbreviation");
Record.clear();
Stream.EmitRecord(llvm::bitc::BLOCKINFO_CODE_SETRECORDNAME, RecordIdNameMap[ID].Name);
```
And after that you can lower the default size of `SmallVector<> Record` down to, hm, `4`?


================
Comment at: clang-doc/BitcodeWriter.h:161
+
+  using RecordData = SmallVector<uint64_t, 128>;
+
----------------
This alias is used exactly once, for `Record` member variable in this class.
Is there any point in having this alias?


================
Comment at: clang-doc/BitcodeWriter.h:161
+
+  using RecordData = SmallVector<uint64_t, 128>;
+
----------------
lebedev.ri wrote:
> This alias is used exactly once, for `Record` member variable in this class.
> Is there any point in having this alias?
Also, why is `uint64_t` used?
We either push `char`, or `enum`, or `int`. Do we ever need 64-bit?


================
Comment at: clang-doc/ClangDoc.h:47
+                       bool OmitFilenames)
+          : Mapper(Ctx, ECtx, OmitFilenames){};
+      void HandleTranslationUnit(clang::ASTContext &Context) override {
----------------
Please add space before `{}`, and drop unneeded `;`


================
Comment at: clang-doc/Mapper.h:56
+ private:
+  class ClangDocCommentVisitor
+      : public ConstCommentVisitor<ClangDocCommentVisitor> {
----------------
`ClangDocMapper` class is staring to look like a god-class.
I would recommend:
1. Rename `ClangDocMapper` to `ClangDocASTVisitor`.
     It's kind-of conventional to name `RecursiveASTVisitor`-based classes like that.
2. Move `ClangDocCommentVisitor` out of the `ClangDocMapper`, into `namespace {}` in `clang-doc/Mapper.cpp`
3. 
      * Split `ClangDocSerializer` into new .h/.cpp
      * Replace `ClangDocSerializer Serializer;` with  `ClangDocSerializer& Serializer;`
      * Instantiate `ClangDocSerializer` (in `MapperActionFactory`, i think?) before `ClangDocMapper`
      * Pass `ClangDocSerializer&` into `ClangDocMapper` ctor.


https://reviews.llvm.org/D41102