lebedev.ri added a comment.
Thank you for working on this!
Some more nitpicking.
//Please// consider adding even more tests (ideally, all this code should have
100% test coverage)
================
Comment at: clang-doc/BitcodeWriter.cpp:139
+ {COMMENT_NAME, {"Name", &StringAbbrev}},
+ {COMMENT_POSITION, {"Position", &IntAbbrev}},
+ {COMMENT_DIRECTION, {"Direction", &StringAbbrev}},
----------------
This change is not covered by tests.
(I've actually found out that the hard way, by trying to find why it didn't
trigger any asssertions, oh well)
================
Comment at: clang-doc/BitcodeWriter.cpp:325
+ emitHeader();
+ Stream.EnterBlockInfoBlock();
+
----------------
I think it would be cleaner to move it (at least the enterblock, it might make
sense to leave the header at the very top) after the static variable
================
Comment at: clang-doc/BitcodeWriter.cpp:363
+
+ for (const auto &Block : TheBlocks) {
+ assert(Block.second.size() < (1U << BitCodeConstants::SubblockIDSize));
----------------
I.e.
```
...
, FUNCTION_IS_METHOD}}};
Stream.EnterBlockInfoBlock();
for (const auto &Block : TheBlocks) {
assert(Block.second.size() < (1U << BitCodeConstants::SubblockIDSize));
emitBlockInfo(Block.first, Block.second);
}
Stream.ExitBlock();
emitVersion();
}
```
================
Comment at: clang-doc/BitcodeWriter.h:19
+
+#include <initializer_list>
+#include <vector>
----------------
Please sort includes, clang-tidy complains.
================
Comment at: clang-doc/BitcodeWriter.h:32
+// BitCodeConstants, though they can be added without breaking it.
+static const unsigned VERSION_NUMBER = 1;
+
----------------
```
/build/clang-tools-extra/clang-doc/BitcodeWriter.h:32:23: warning: invalid case
style for variable 'VERSION_NUMBER' [readability-identifier-naming]
static const unsigned VERSION_NUMBER = 1;
^~~~~~~~~~~~~~
VersionNumber
```
================
Comment at: clang-doc/BitcodeWriter.h:163
+ public:
+ ClangDocBitcodeWriter(llvm::BitstreamWriter &Stream,
+ bool OmitFilenames = false)
----------------
The simplest solution would be
```
#ifndef NDEBUG // Don't want explicit dtor unless needed
~ClangDocBitcodeWriter() {
// Check that the static size is large-enough.
assert(Record.capacity() == BitCodeConstants::RecordSize);
}
#endif
```
================
Comment at: clang-doc/BitcodeWriter.h:228
+ // Static size is the maximum length of the block/record names we're pushing
+ // to this + 1. Longest is currently `MemberTypeBlock` at 15 chars.
+ SmallVector<uint32_t, BitCodeConstants::RecordSize> Record;
----------------
So you want to be really definitive with this. I wanted to avoid that,
actually..
Then i'm afraid one more assert is needed, to make sure this is *actually* true.
I'm not seeing any way to make `SmallVector` completely static,
so you could either add one more wrapper around it (rather ugly),
or check the final size in the `ClangDocBitcodeWriter` destructor (will not
pinpoint when the size has 'overflowed')
================
Comment at: clang-doc/BitcodeWriter.h:246
+void ClangDocBitcodeWriter::writeBitstream(const T &I, bool WriteBlockInfo) {
+ if (WriteBlockInfo) emitBlockInfoBlock();
+ StreamSubBlockGuard Block(Stream, MapFromInfoToBlockId<T>::ID);
----------------
Does it *ever* make sense to output `BlockInfoBlock` anywhere else other than
once at the very beginning?
I'd think you should drop the boolean param, and unconditinally call the
`emitBlockInfoBlock();` from `ClangDocBitcodeWriter::ClangDocBitcodeWriter()`
ctor.
================
Comment at: clang-doc/BitcodeWriter.h:248
+ StreamSubBlockGuard Block(Stream, MapFromInfoToBlockId<T>::ID);
+ emitBlock(I);
+}
----------------
The naming choices confuse me.
There is `writeBitstream()` and `emitBlock()`, which is called from
`writeBitstream()` to write the actual contents of the block.
Why one is `write` and another is `emit`?
To match the `BitstreamWriter` naming choices? (which uses `Emit` prefix)?
To avoid the confusion of which one outputs the actual content, and which one
outputs the whole block?
I think it should be:
*
```
- void emitBlock(const NamespaceInfo &I);
+ void emitBlockContent(const NamespaceInfo &I);
```
*
```
- void ClangDocBitcodeWriter::writeBitstream(const T &I, bool WriteBlockInfo);
+ void ClangDocBitcodeWriter::emitBlock(const T &I, bool EmitBlockInfo);
```
This way, *i think* their names would clearner-er state what they do, and won't
be weirdly different.
What do you think?
================
Comment at: clang-doc/Representation.h:18
+
+#include <string>
+#include "clang/AST/Type.h"
----------------
Please sort includes, clang-tidy complains.
================
Comment at: clang-doc/Serialize.cpp:88
+ CurrentCI.Name = getCommandName(C->getCommandID());
+ for (unsigned i = 0, e = C->getNumArgs(); i < e; ++i)
+ CurrentCI.Args.push_back(C->getArgText(i));
----------------
```
/build/clang-tools-extra/clang-doc/Serialize.cpp:88:17: warning: invalid case
style for variable 'i' [readability-identifier-naming]
for (unsigned i = 0, e = C->getNumArgs(); i < e; ++i)
^ ~ ~~
I I I
/build/clang-tools-extra/clang-doc/Serialize.cpp:88:24: warning: invalid case
style for variable 'e' [readability-identifier-naming]
for (unsigned i = 0, e = C->getNumArgs(); i < e; ++i)
^ ~~
E E
```
================
Comment at: clang-doc/Serialize.cpp:107
+ if (C->isPositionValid()) {
+ for (unsigned i = 0, e = C->getDepth(); i < e; ++i)
+ CurrentCI.Position.push_back(C->getIndex(i));
----------------
```
/build/clang-tools-extra/clang-doc/Serialize.cpp:107:19: warning: invalid case
style for variable 'i' [readability-identifier-naming]
for (unsigned i = 0, e = C->getDepth(); i < e; ++i)
^ ~ ~~
I I I
/build/clang-tools-extra/clang-doc/Serialize.cpp:107:26: warning: invalid case
style for variable 'e' [readability-identifier-naming]
for (unsigned i = 0, e = C->getDepth(); i < e; ++i)
^ ~~
E E
```
================
Comment at: clang-doc/Serialize.h:19
+
+#include <string>
+#include <vector>
----------------
Please sort includes, clang-tidy complains.
================
Comment at: clang-doc/tool/ClangDocMain.cpp:80
+ getInsertArgumentAdjuster("-fparse-all-comments",
+ tooling::ArgumentInsertPosition::BEGIN),
+ ArgAdjuster);
----------------
Why at the beginning though?
Couldn't the user pass `-extra-arg=-fno-parse-all-comments`, which could
override this?
https://reviews.llvm.org/D41102
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
