[PATCH] D41102: Setup clang-doc frontend framework

[Roman Lebedev via Phabricator via cfe-commits]

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