lebedev.ri added a comment.
Great work!
Some more review...
================
Comment at: clang-doc/ClangDoc.cpp:32
+ ECtx.reportResult(
+ Name, Mapper.emitInfo(D, getComment(D), Name, getLine(D), getFile(D)));
+}
----------------
I wonder if `Name` should be `std::move()`'d ? Or not, `reportResult()` seems
to take `StringRef`...
(in general, it might be a good idea to run clang-tidy on the code)
================
Comment at: clang-doc/ClangDoc.h:46
+ void processMatchedDecl(const T *D);
+ int getLine(const NamedDecl *D) const;
+ StringRef getFile(const NamedDecl *D) const;
----------------
//I// would add an empty line:
```
private:
template <class T>
void processMatchedDecl(const T *D);
int getLine(const NamedDecl *D) const;
StringRef getFile(const NamedDecl *D) const;
comments::FullComment *getComment(const NamedDecl *D) const;
std::string getName(const NamedDecl *D) const;
```
================
Comment at: clang-doc/ClangDocBinary.cpp:72
+ assert(Abbrevs.find(recordID) == Abbrevs.end() &&
+ "Abbreviation already set.");
+ Abbrevs[recordID] = abbrevID;
----------------
lebedev.ri wrote:
> So it does not *set* the abbreviation, since it is not supposed to be called
> if the abbreviation is already set, but it *adds* a unique abbreviation.
> I think it should be called `void AbbreviationMap::add(unsigned recordID,
> unsigned abbrevID)` then
This is marked as done, but the name is still the same, and no counter-comment
was added, as far as i can see
================
Comment at: clang-doc/ClangDocBinary.cpp:88
+ Stream.Emit((unsigned)'C', 8);
+ Stream.Emit((unsigned)'S', 8);
+}
----------------
juliehockett wrote:
> lebedev.ri wrote:
> > General comment: shouldn't the bitcode be versioned?
> Possibly? My understanding of the versioning (which could be incorrect) was
> that it was for the LLVM IR and how it is written in the given file -- I'm
> not writing to LLVM IR here, just using it as a data storage format, and so
> didn't think it was necessary. Happy to add it in though, but which version
> number should I use?
The question i'm asking is: what will happen if two different (documenting
different attributes, with non-identical `enum {something}Id`, etc) clang-doc's
were used to generate two different parts of the docs (two different TU's)?
When merging two parts, if the older clang-doc is used, will it only accept the
part if bc it understands? Or fail altogether?
And, does it make sense to allow to generate such mixed-up documentation?
================
Comment at: clang-doc/ClangDocBinary.cpp:30
+unsigned AbbreviationMap::get(RecordId RID) {
+ assert(Abbrevs.find(RID) != Abbrevs.end() && "Abbreviation not added.");
+ return Abbrevs[RID];
----------------
Maybe `"Unknown Abbreviation."` ?
================
Comment at: clang-doc/ClangDocBinary.cpp:63
+
+// Common Abbreviations
+
----------------
So, these `emitStringAbbrev()`, `emitLocationAbbrev()` and `emitIntAbbrev()`
are quite similar.
How about **something** like:
```
template <typename Lambda>
void ClangDocBinaryWriter::emitAbbrev(RecordId ID, BlockId Block, Lambda &&L,
&Stream) {
auto Abbrev = std::make_shared<BitCodeAbbrev>();
Abbrev->Add(BitCodeAbbrevOp(ID));
L(Abbrev);
Abbrevs.add(ID, Stream.EmitBlockInfoAbbrev(Block, std::move(Abbrev)));
}
void ClangDocBinaryWriter::emitStringAbbrev(RecordId ID, BlockId Block,
BitstreamWriter &Stream) {
auto EmitString = [](std::shared_ptr<BitCodeAbbrev> &Abbrev) {
Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed,
BitCodeConstants::LineNumFixedSize)); // String size
Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob)); // String
};
emitAbbrev(ID, Block, EmitString, Stream);
}
...
```
?
================
Comment at: clang-doc/ClangDocBinary.cpp:69
+ Abbrev->Add(BitCodeAbbrevOp(ID));
+ Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed,
BitCodeConstants::LineNumFixedSize)); // String size
+ Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob)); // String
----------------
And that de-magicking made these strings longer than `80` columns, boo :(
================
Comment at: clang-doc/ClangDocBinary.cpp:71
+ Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob)); // String
+ Abbrevs.add(ID, Stream.EmitBlockInfoAbbrev(Block, Abbrev));
+}
----------------
I think you should `std::move()` the `Abbrev`, it's not used afterwards in this
function anyways,
and it //seems// to generate nicer code https://godbolt.org/g/ow58JV
================
Comment at: clang-doc/ClangDocBinary.cpp:132
+ emitIntRecord(N.Access, NAMED_TYPE_ACCESS, Stream);
+ Stream.ExitBlock();
+}
----------------
There is a common patter here;
I'd try to do something like:
```
class StreamSubBlock {
BitstreamWriter &Stream;
public:
StreamSubBlock(BitstreamWriter &Stream_, BlockId ID) : Stream(Stream_) {
Stream.EnterSubblock(ID, BitCodeConstants::SubblockIDSize);
}
// Optionally, also delete all the other constructors / copy/move operators.
~StreamSubBlock() {
Stream.ExitBlock();
}
}
void ClangDocBinaryWriter::emitNamedTypeBlock(const NamedType &N,
NamedType::FieldName ID,
BitstreamWriter &Stream) {
StreamSubBlock Block(Stream, NAMED_TYPE_BLOCK_ID);
emitIntRecord(ID, NAMED_TYPE_ID, Stream);
emitStringRecord(N.Type, NAMED_TYPE_TYPE, Stream);
emitStringRecord(N.Name, NAMED_TYPE_NAME, Stream);
emitIntRecord(N.Access, NAMED_TYPE_ACCESS, Stream);
}
```
I.e. use RAII
================
Comment at: clang-doc/ClangDocBinary.h:82
+
+static std::map<BlockId, std::string> BlockIdNameMap = {
+ {NAMESPACE_BLOCK_ID, "NamespaceBlock"},
----------------
Nice!
Some thoughts:
1. I agree it makes sense to keep it close to the enum definition, in header...
2. This will result in global constructor. Generally they are frowned upon in
LLVM. But since this is a standalone binary, it may be ok?
3. Have you tried using `StringRef` here, instead of `std::string`?
4. `std::map` is in general a bad idea.
Since the `enum`'s enumerators are all small and consecutive, maybe try
`llvm::IndexedMap`?
================
Comment at: clang-doc/ClangDocBinary.h:92
+
+static std::map<RecordId, std::string> RecordIdNameMap = {
+ {COMMENT_KIND, "Kind"},
----------------
Same as in the previous note ^
================
Comment at: clang-doc/ClangDocBinary.h:176
+ void emitBlockID(BlockId ID, BitstreamWriter &Stream);
+ void emitStringAbbrev(RecordId ID, BlockId Block, BitstreamWriter &Stream);
+ void emitLocationAbbrev(RecordId ID, BlockId Block, BitstreamWriter &Stream);
----------------
i'd add an empty line before `void emitStringAbbrev(RecordId ID, BlockId Block,
BitstreamWriter &Stream);`
================
Comment at: clang-doc/ClangDocMapper.cpp:202
+ for (comments::Comment *Child :
+ make_range(C->child_begin(), C->child_end())) {
+ CurrentCI.Children.emplace_back(std::make_shared<CommentInfo>());
----------------
It would be nice if you could (as a new Differential) add a `children()`
function to that class that will do that automatically.
================
Comment at: clang-doc/ClangDocMapper.cpp:216
+ CurrentCI.Name = getCommandName(C->getCommandID());
+ for (unsigned i = 0, e = C->getNumArgs(); i != e; ++i)
+ CurrentCI.Args.push_back(C->getArgText(i));
----------------
It would be **awesome** if you could (as a new Differential) add a
`args_begin()`/`args_end()`/`args()` functions for range-based `for`.
================
Comment at: clang-doc/ClangDocMapper.cpp:224
+ CurrentCI.SelfClosing = C->isSelfClosing();
+ for (unsigned i = 0, e = C->getNumAttrs(); i < e; ++i) {
+ const HTMLStartTagComment::Attribute &Attr = C->getAttr(i);
----------------
It would be **awesome** if you could (as a new Differential) add a
`attrs_begin()`/`attrs_end()`/`attrs()` functions for range-based `for`.
================
Comment at: clang-doc/ClangDocMapper.cpp:240
+ CurrentCI.Name = getCommandName(C->getCommandID());
+ for (unsigned i = 0, e = C->getNumArgs(); i < e; ++i)
+ CurrentCI.Args.push_back(C->getArgText(i));
----------------
It would be **awesome** if you could (as a new Differential) add a
`args_begin()`/`args_end()`/`args()` functions for range-based `for`.
================
Comment at: clang-doc/ClangDocMapper.cpp:259
+ if (C->isPositionValid()) {
+ for (unsigned i = 0, e = C->getDepth(); i < e; ++i)
+ CurrentCI.Position.push_back(std::to_string(C->getIndex(i)));
----------------
It would be **awesome** if you could (as a new Differential) add a
`positions_begin()`/`positions_end()`/`positions()` functions for range-based
`for`.
================
Comment at: docs/clang-doc.rst:57
+ -doxygen - Use only doxygen-style comments to
generate docs.
+ -dump - Dump results to stdout.
+ -extra-arg=<string> - Additional argument to append to the
compiler command line
----------------
Please refresh the doc to actually show the `clang-doc` current help output.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D41102
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
