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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
