[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Eric Liu via Phabricator via cfe-commits]

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

lgtm with just a few more nits.



================
Comment at: clang-doc/BitcodeWriter.cpp:484
 
 #undef EMITINFO
 
----------------
Nit: `EMITINFO` is a bit confusing with `writeInfo`. Are they doing the same thing (`emit` sounds like `write`)? You might want to rename either of them. While you are here, it might also make sense to clear up the MACRO :) 


================
Comment at: clang-doc/Representation.cpp:57
+    return llvm::make_error<llvm::StringError>("Unexpected info type.\n",
+                                               std::error_code());
+  }
----------------
nit: `std::error_code()` is usually used to indicate no error. You could use `llvm::inconvertibleErrorCode()`.


================
Comment at: clang-doc/tool/ClangDocMain.cpp:181
+        doc::writeInfo(I.get(), Buffer);
+      if (DumpResultToFile("bc", Group.getKey() + ".bc", Buffer))
+        return 1;
----------------
juliehockett wrote:
> ioeric wrote:
> > juliehockett wrote:
> > > ioeric wrote:
> > > > (Sorry that I might be missing context here.)
> > > > 
> > > > Could you please explain the incentive for dumping each info group to one bc file? If the key identifies a symbol (e.g. USR), wouldn't this result in a bitcode file being created for each symbol? This doesn't seem very scalable.  
> > > Yes, it would. This is mostly for debugging, since there's not really any tools outside the clang system that would actually want/be able to use this information.
> > Ok, is there any plan to convert intermediate result to final result and output in a more accessible format? If so, please add a `FIXME` somewhere just to be clearer.
> That's what the next patch set is (to generate YAML).
Sure. I think a `FIXME` should be in place if the feature is not already there.


================
Comment at: clang-doc/tool/ClangDocMain.cpp:68
+    "format",
+    llvm::cl::desc("Format for outputted docs (Current options are yaml)."),
+    llvm::cl::init("yaml"), llvm::cl::cat(ClangDocCategory));
----------------
nit: s/current option/default option/


================
Comment at: clang-doc/tool/ClangDocMain.cpp:68
+    "format",
+    llvm::cl::desc("Format for outputted docs (Current options are yaml)."),
+    llvm::cl::init("yaml"), llvm::cl::cat(ClangDocCategory));
----------------
ioeric wrote:
> nit: s/current option/default option/
consider using enum type options. Example: https://github.com/llvm-mirror/clang-tools-extra/blob/master/include-fixer/tool/ClangIncludeFixer.cpp#L85


================
Comment at: clang-doc/tool/ClangDocMain.cpp:178
+    if (!Reduced)
+      llvm::errs() << llvm::toString(Reduced.takeError());
+
----------------
Shouldn't we `continue` to the next group if reduction goes wrong for the current group?


https://reviews.llvm.org/D43341