[PATCH] D41102: Setup clang-doc frontend framework
Roman Lebedev via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Feb 21 09:06:49 PST 2018
lebedev.ri added a comment.
Nice!
================
Comment at: clang-doc/BitcodeWriter.cpp:33
+ llvm::IndexedMap<StringRef, BlockIdToIndexFunctor> BlockIdNameMap;
+ BlockIdNameMap.resize(BI_LAST - BI_FIRST + 1);
+
----------------
So here's the thing.
We know how many enumerators we have (`BI_LAST - BI_FIRST + 1`).
And we know how many enumerators we will init (`inits.size()`).
Those numbers should match.
================
Comment at: clang-doc/BitcodeWriter.cpp:47
+ {BI_COMMENT_BLOCK_ID, "CommentBlock"}};
+ for (const auto &init : inits) {
+ BlockIdNameMap[init.first] = init.second;
----------------
Can elide `{}`
================
Comment at: clang-doc/BitcodeWriter.cpp:50
+ }
+ return BlockIdNameMap;
+ }();
----------------
So i would recommend:
```
static constexpr unsigned ExpectedSize = BI_LAST - BI_FIRST + 1;
BlockIdNameMap.resize(ExpectedSize);
...
static_assert(inits.size() == ExpectedSize, "unexpected count of initializers");
for (const auto &init : inits)
BlockIdNameMap[init.first] = init.second;
assert(BlockIdNameMap.size() == ExpectedSize);
```
================
Comment at: clang-doc/BitcodeWriter.cpp:56
+ llvm::IndexedMap<StringRef, RecordIdToIndexFunctor> RecordIdNameMap;
+ RecordIdNameMap.resize(RI_LAST - RI_FIRST + 1);
+
----------------
Same
So the differences between the two are:
* functors
* `*_LAST` and `*_FIRST` params
* the actual initializer-list
I guess it might be //eventually// refactored as new `llvm::IndexedMap` ctor.
================
Comment at: clang-doc/BitcodeWriter.cpp:120
+void ClangDocBinaryWriter::emitHeader(BitstreamWriter &Stream) {
+ // Emit the file header.
+ Stream.Emit((unsigned)'D', BitCodeConstants::SignatureBitSize);
----------------
I wonder if this would work?
```
for(char c : StringRef("DOCS"))
Stream.Emit((unsigned)c, BitCodeConstants::SignatureBitSize);
```
================
Comment at: clang-doc/BitcodeWriter.cpp:258
+ StreamSubBlock Block(Stream, BI_COMMENT_BLOCK_ID);
+ emitStringRecord(I->Text, COMMENT_TEXT, Stream);
+ emitStringRecord(I->Name, COMMENT_NAME, Stream);
----------------
Hmm, you could try something like
```
for(const auto& L : std::initializer_list<std::pair<StringRef, RecordId>>{{I->Text, COMMENT_TEXT}, {I->Name, COMMENT_NAME}, ...})
emitStringRecord(L.first, S.second, Stream);
```
================
Comment at: clang-doc/BitcodeWriter.cpp:286
+ emitBlockID(BI_COMMENT_BLOCK_ID, Stream);
+ emitRecordID(COMMENT_KIND, Stream);
+ emitRecordID(COMMENT_TEXT, Stream);
----------------
```
for(RecordId RID : {COMMENT_KIND, COMMENT_TEXT, COMMENT_NAME, COMMENT_DIRECTION,
COMMENT_PARAMNAME, COMMENT_CLOSENAME, COMMENT_SELFCLOSING,
COMMENT_EXPLICIT, COMMENT_ATTRKEY, COMMENT_ATTRVAL, COMMENT_ARG,
COMMENT_POSITION})
emitRecordID(RID, Stream);
```
should work
================
Comment at: clang-doc/BitcodeWriter.cpp:298
+ emitRecordID(COMMENT_POSITION, Stream);
+ emitStringAbbrev(COMMENT_KIND, BI_COMMENT_BLOCK_ID, Stream);
+ emitStringAbbrev(COMMENT_TEXT, BI_COMMENT_BLOCK_ID, Stream);
----------------
```
for(RecordId RID : {COMMENT_KIND, COMMENT_TEXT, COMMENT_NAME, COMMENT_DIRECTION,
COMMENT_PARAMNAME, COMMENT_CLOSENAME})
emitStringAbbrev(RID, BI_COMMENT_BLOCK_ID, Stream);
```
================
Comment at: clang-doc/BitcodeWriter.cpp:306
+ emitIntAbbrev(COMMENT_EXPLICIT, BI_COMMENT_BLOCK_ID, Stream);
+ emitStringAbbrev(COMMENT_ATTRKEY, BI_COMMENT_BLOCK_ID, Stream);
+ emitStringAbbrev(COMMENT_ATTRVAL, BI_COMMENT_BLOCK_ID, Stream);
----------------
```
for(RecordId RID : {COMMENT_ATTRKEY, COMMENT_ATTRVAL, COMMENT_ARG, COMMENT_POSITION})
emitStringAbbrev(RID, BI_COMMENT_BLOCK_ID, Stream);
```
and maybe in a few other places
================
Comment at: clang-doc/BitcodeWriter.cpp:407
+
+void ClangDocBinaryWriter::writeBitstream(const EnumInfo &I,
+ BitstreamWriter &Stream,
----------------
Hmm, common pattern again
```
void ClangDocBinaryWriter::writeBitstream(const <TYPENAME> &I,
BitstreamWriter &Stream,
bool writeBlockInfo) {
if (writeBlockInfo) emitBlockInfoBlock(Stream);
StreamSubBlock Block(Stream, BI_<ENUM_NAMETYPE>_BLOCK_ID);
...
}
```
Could be solved if a mapping from `TYPENAME` -> `BI_<ENUM_NAMETYPE>_BLOCK_ID` can be added.
If LLVM would be using C++14, that'd be easy, but with C++11, it would require whole new class (although with just a single static variable).
================
Comment at: clang-doc/BitcodeWriter.h:11
+// This file implements a writer for serializing the clang-doc internal
+// represeentation to LLVM bitcode. The writer takes in a stream and emits the
+// generated bitcode to that stream.
----------------
representation
================
Comment at: clang-doc/BitcodeWriter.h:32
+
+#define VERSION_NUMBER 0
+
----------------
`static const unsigned` please :)
I'm not sure where to best to keep it, in anon namespace, here, or in `BitCodeConstants` struct.
Probably here, at least for now.
Also, a general comment is needed, on when to bump it (when changing `BitCodeConstants`/`BlockId`/`RecordId`, ...)
================
Comment at: clang-doc/BitcodeWriter.h:54
+
+#define INFORECORDS(X) X##_NAME, X##_NAMESPACE,
+
----------------
I'd try removing that last comma, and manually adding it after the macro instantiation each time, i.e.
```
#define INFORECORDS(X) X##_NAME, X##_NAMESPACE
...
enum RecordId {
...
MEMBER_TYPE_ACCESS,
INFORECORDS(NAMESPACE),
INFORECORDS(ENUM),
ENUM_ISDEFINITION,
...
};
```
Maybe that is better for `clang-format`? Or is the current version intentional?
================
Comment at: clang-doc/BitcodeWriter.h:95
+
+class ClangDocBinaryWriter {
+ public:
----------------
High-level remark about `ClangDocBinaryWriter` class.
* Pretty much all of it's member functions take `BitstreamWriter &Stream` parameter.
* `ClangDocBinaryWriter` is a member variable `Writer` in `ClangDocSerializer`.
* As far as i can tell, it (`Writer`) is only used in one function - `template <typename T> std::string ClangDocMapper::ClangDocSerializer::serialize(T &I)`. However, i don't really understand whether that function will be called more than once or not. I guess it will?
I'd recommend to refactor this somehow, so you don't have to always pass the `BitstreamWriter &Stream` each time.
The easiest, 'least intrusive' solution, i guess, would be adding `BitstreamWriter *Stream` as member variable of `ClangDocBinaryWriter`, and setting/unsetting it in `ClangDocMapper::ClangDocSerializer::serialize()`.
Or maybe you can split `ClangDocBinaryWriter` somehow, so it can be constructed (in `ClangDocMapper::ClangDocSerializer::serialize()`) with `BitstreamWriter &Stream`?
Or better yet, couldn't you just move `ClangDocBinaryWriter Writer` variable into the `ClangDocMapper::ClangDocSerializer::serialize()`, and construct it each time? (with `BitstreamWriter &Stream`)
================
Comment at: clang-doc/BitcodeWriter.h:98
+ ClangDocBinaryWriter(bool OmitFilenames = false)
+ : OmitFilenames(OmitFilenames){};
+
----------------
Extra `;`
Space before `{}`
================
Comment at: clang-doc/BitcodeWriter.h:118
+ void add(RecordId RID, unsigned abbrevID);
+ unsigned get(RecordId RID);
+ void clear();
----------------
Can `AbbreviationMap::get(RecordId RID)` be `const` method?
================
Comment at: clang-doc/BitcodeWriter.h:122
+
+ class StreamSubBlock {
+ BitstreamWriter &Stream;
----------------
I know this is the name i used in my snippet, but i wonder if some name that better relays the fact that it is a RAII-type class would be better?
`StreamSubBlockGuard` ?
================
Comment at: clang-doc/ClangDoc.h:42
+
+ clang::FrontendAction *create() override {
+ class ClangDocConsumer : public clang::ASTConsumer {
----------------
Eww, `tooling::FrontendActionFactory::create()` is supposed to return owning pointer, not `std::unique_ptr<>` :/
================
Comment at: clang-doc/ClangDoc.h:48
+ : Mapper(Ctx, ECtx, OmitFilenames){};
+ virtual void HandleTranslationUnit(clang::ASTContext &Context) {
+ Mapper.TraverseDecl(Context.getTranslationUnitDecl());
----------------
s/virtual/override/ ?
================
Comment at: clang-doc/ClangDoc.h:61
+
+ virtual std::unique_ptr<clang::ASTConsumer> CreateASTConsumer(
+ clang::CompilerInstance &Compiler, llvm::StringRef InFile) {
----------------
s/virtual/override/ ?
================
Comment at: clang-doc/Mapper.cpp:113
+ populateInfo(I, D, C);
+ I.Loc.emplace_back(Location{LineNumber, File});
+}
----------------
```
I.Loc.emplace_back({LineNumber, File});
```
================
Comment at: clang-doc/Representation.h:49
+ llvm::SmallVector<std::string, 4> Position;
+ std::vector<CommentInfo *> Children;
+};
----------------
`std::vector<std::unique_ptr<CommentInfo>> Children;` please
https://reviews.llvm.org/D41102
More information about the cfe-commits
mailing list