[PATCH] D41102: Setup clang-doc frontend framework

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

There's a few places where we can trim some of the boilerplate, which I think is important - it's hard to find the "real code" among all the plumbing in places.
Other than that, this seems OK to me.



================
Comment at: clang-doc/BitcodeWriter.h:116
+template <typename Info> struct MapFromInfoToBlockId {
+  static const BlockId ID;
+};
----------------
I think you don't want to declare ID in the unspecialized template, so you get a compile error if you try to use it.

(Using traits for this sort of thing seems a bit overboard to me, but YMMV)


================
Comment at: clang-doc/BitcodeWriter.h:154
+  ClangDocBitcodeWriter(llvm::BitstreamWriter &Stream,
+                        bool OmitFilenames = false)
+      : Stream(Stream), OmitFilenames(OmitFilenames) {
----------------
Hmm, you spend a lot of effort plumbing this variable around! Why is it so important?
Filesize? (I'm not that familiar with LLVM bitcode, but surely we'll end up with a string table anyway?)

If it really is an important option people will want, the command-line arg should probably say why.


================
Comment at: clang-doc/BitcodeWriter.h:241
+/// \param I The info to emit to bitcode.
+template <typename T> void ClangDocBitcodeWriter::emitBlock(const T &I) {
+  StreamSubBlockGuard Block(Stream, MapFromInfoToBlockId<T>::ID);
----------------
OK, I don't get this at all.

We have to declare emitBlockContent(NamespaceInfo) *and* the specialization of MapFromInfoToBlockId<NamespaceInfo>, and deal with the public interface emitBlock being a template function where you can't tell what's legal to pass, instead of writing:

```void emitBlock(const NamespaceInfo &I) {
  SubStreamBlockGuard Block(Stream, BI_NAMESPACE_BLOCK_ID); // <-- this one line
  ...
}```

This really seems like templates for the sake of templates :(


================
Comment at: clang-doc/ClangDoc.h:10
+//
+// This file implements the main entry point for the clang-doc tool. It runs
+// the clang-doc mapper on a given set of source code files using a
----------------
This comment doesn't seem accurate - there's no main() in this file.
There's a FrontendActionFactory, but nothing in this file uses it.


================
Comment at: clang-doc/ClangDoc.h:37
+
+  clang::FrontendAction *create() override {
+    class ClangDocConsumer : public clang::ASTConsumer {
----------------
nit: seems odd to put all this implementation in the header.
(personally I'd just expose a function returning unique_ptr<FrontendActionFactory> from the header, but up to you...)


================
Comment at: clang-doc/ClangDoc.h:38
+  clang::FrontendAction *create() override {
+    class ClangDocConsumer : public clang::ASTConsumer {
+    public:
----------------
for ASTConsumers implemented by ASTVisitors, there seems a fairly strong convention to just make the same class extend both (MapASTVisitor, here).
That would eliminate one plumbing class...


================
Comment at: clang-doc/Mapper.cpp:33
+  ECtx->reportResult(llvm::toHex(llvm::toStringRef(serialize::hashUSR(USR))),
+                     serialize::emitInfo(D, getComment(D, D->getASTContext()),
+                                         getLine(D, D->getASTContext()),
----------------
It seems a bit of a poor fit to use a complete bitcode file (header, version, block info) as your value format when you know the format, and know there'll be no version skew.
Is it easy just to emit the block we care about?


================
Comment at: clang-doc/Representation.h:29
+
+using USRString = std::array<uint8_t, 20>;
+
----------------
lebedev.ri wrote:
> Right, of course, internally this is kept in the binary format, which is just 20 chars.
> This is not the string (the hex-ified version of sha1), but the raw sha1, the binary.
> This should somehow convey that. This should be something closer to `USRSha1`.
I'm not sure that any of the implementation (either USR or SHA) belongs in the type name.
In clangd we called this type SymbolID, which seems like a reasonable name here too.


================
Comment at: clang-doc/Representation.h:44
+  CommentInfo(CommentInfo &&Other) : Children(std::move(Other.Children)) {}
+  SmallString<16> Kind;
+  SmallString<64> Text;
----------------
this is probably the right place to document these fields - what are the legal kinds? what's the name of a comment, direction, etc?


https://reviews.llvm.org/D41102