lebedev.ri added a comment.
Nice work!
It will be great to have a replacement for doxygen, which is actually modern
and usable.
Some review notes for some of the code below:
================
Comment at: clang-doc/ClangDocBinary.cpp:17
+
+enum BlockIds {
+ NAMESPACE_BLOCK_ID = bitc::FIRST_APPLICATION_BLOCKID,
----------------
I wonder if you could add a map from `BlockIds` enumerator to the textual
representation.
e.g. `NAMESPACE_BLOCK_ID` -> "NamespaceBlock"
...
`RECORD_BLOCK_ID` -> "RecordBlock"
...
This would allow to only pass the `BlockId`, and avoid passing hardcoded string
each time.
================
Comment at: clang-doc/ClangDocBinary.cpp:72
+ assert(Abbrevs.find(recordID) == Abbrevs.end() &&
+ "Abbreviation already set.");
+ Abbrevs[recordID] = abbrevID;
----------------
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
================
Comment at: clang-doc/ClangDocBinary.cpp:88
+ Stream.Emit((unsigned)'C', 8);
+ Stream.Emit((unsigned)'S', 8);
+}
----------------
General comment: shouldn't the bitcode be versioned?
================
Comment at: clang-doc/ClangDocBinary.cpp:99
+ // Emit the block name if present.
+ if (!Name || Name[0] == 0) return;
+ Record.clear();
----------------
So you are actually checking that there is either no string, or the string is
of zero length here.
Is this function ever going to be called with a null `Name`?
All calls in this Differential always pass a static C string here.
Also see my comments about passing enumerator, and having a map that would
avoid passing string altogether.
================
Comment at: clang-doc/ClangDocBinary.cpp:120
+ Abbrev->Add(BitCodeAbbrevOp(D));
+ Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 16)); // String size
+ Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob)); // String
----------------
These constants are somewhat vague.
Maybe consolidate them somewhere somehow, e.g.:
```
clang-doc/ClangDocBinary.cpp:
namespace {
struct BitCodeConstants {
static constexpr int LineNumFixedSize = 16;
...
}
}
```
================
Comment at: clang-doc/ClangDocBinary.cpp:178
+ BitstreamWriter &Stream) {
+ Stream.EnterSubblock(NAMED_TYPE_BLOCK_ID, 5);
+ emitIntRecord(ID, NAMED_TYPE_ID, Stream);
----------------
From the docs i can see that `5` is `CodeLen`, but how is this decided, etc?
Seems like a magical constant, maybe consolidate them somewhere, like in the
previous note?
================
Comment at: clang-doc/ClangDocBinary.h:57
+
+ void emitRecordID(unsigned ID, const char *Name, BitstreamWriter &Stream);
+ void emitBlockID(unsigned ID, const char *Name, BitstreamWriter &Stream);
----------------
Should these take `StringRef` instead of `const char *` ?
================
Comment at: clang-doc/ClangDocBinary.h:57
+
+ void emitRecordID(unsigned ID, const char *Name, BitstreamWriter &Stream);
+ void emitBlockID(unsigned ID, const char *Name, BitstreamWriter &Stream);
----------------
lebedev.ri wrote:
> Should these take `StringRef` instead of `const char *` ?
>
Also, isn't the first param always a `BlockIds`? Why not pass enumerators, and
make it more obvious?
================
Comment at: clang-doc/ClangDocBinary.h:62
+ void emitIntAbbrev(unsigned D, unsigned Block, BitstreamWriter &Stream);
+
+ RecordData Record;
----------------
^ I think all these `unsigned Block` is actually a `BlockIds Block` ?
And `unsigned D` is actually `DataTypes D` ?
================
Comment at: clang-doc/tool/ClangDocMain.cpp:41
+static cl::opt<std::string> OutDirectory(
+ "docs", cl::desc("Directory for outputting docs."), cl::init("docs"),
+ cl::cat(ClangDocCategory));
----------------
Hmm, are you sure about `docs` being the param to specify where to output the
docs?
I'd //expect// to see `-o / --output` or a positional argument.
Or is that impossible due to some parent LLVM/clang implicit requirements?
================
Comment at: clang-doc/tool/ClangDocMain.cpp:45
+static cl::opt<std::string> Format(
+ "format", cl::desc("Format for outputting docs. (options are md)"),
+ cl::init("md"), cl::cat(ClangDocCategory));
----------------
`options are: md`
Though this appears to be a dead code right now
================
Comment at: clang-doc/tool/ClangDocMain.cpp:97
+ // Mapping phase
+ errs() << "Mapping decls...\n";
+ auto Err =
----------------
This does not seem to be a erroneous situation to be in
================
Comment at: clang-doc/tool/ClangDocMain.cpp:107
+ sys::path::native(OutDirectory, IRRootPath);
+ std::error_code DirectoryStatus =
sys::fs::create_directories(IRRootPath);
+ if (DirectoryStatus != OK) {
----------------
I'm having trouble following.
`DumpResult` description says `Dump results to stdout.`
Why does it need `OutDirectory`?
================
Comment at: docs/clang-doc.rst:37
+As currently implemented, the tool is only able to parse TUs that can be
+stored in-memory. Future additions will extend the current framewwork to use
+map-reduce frameworks to allow for use with large codebases.
----------------
framework
================
Comment at: docs/clang-doc.rst:44
+
+ $ clang-doc --help
+ USAGE: clang-doc [options] <source0> [... <sourceN>]
----------------
This does not seem to talk about the path where to store the generated docs.
https://reviews.llvm.org/D41102
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
