[PATCH] D43667: [clang-doc] Implement a YAML generator

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: clang-doc/Generators.h:25
+//
+// Derived classes must implement the generate*ForInfo methods, which should
+// emit documentation for the specified info in the relevant format. This is
----------------
nit: This seems a bit redundant as the virtual method has been set to 0. 


================
Comment at: clang-doc/Generators.h:49
+
+static GeneratorRegistry::Add<YAMLGenerator> YAML(YAMLGenerator::Format,
+                                                  "Generator for YAML output.");
----------------
I think this should go into the cpp file, and you might need the anchor source/destination trick to force the plugin to be linked. See example:
https://github.com/llvm-mirror/clang/blob/master/lib/Tooling/StandaloneExecution.cpp#L88
https://github.com/llvm-mirror/clang/blob/master/lib/Tooling/Execution.cpp#L111

 `YAMLGenerator` could potentially also live in cpp file if you don't expect it to be used directly. 


================
Comment at: clang-doc/generators/YAMLGenerator.cpp:223
+
+template <> struct MappingTraits<std::unique_ptr<CommentInfo>> {
+  static void mapping(IO &IO, std::unique_ptr<CommentInfo> &I) {
----------------
juliehockett wrote:
> ioeric wrote:
> > YAML mapping for `unique_ptr` is a bit unusual. I wonder whether this would work all the time e.g. if the unique_ptr has not been allocated. 
> Mmm yes it's a little weird -- that said, is there a better way emit the info given a pointer?
Not sure what the initial intention was for using `unique_ptr` here, but one option is to get rid of the `unique_ptr` and just store `CommentInfo` in the structure. Another (less ideal) option is to check whether `I` is nullptr and allocate when it's null (only when deserialization though; I think you could tell this from `IO`); and you'd probably want to avoid the allocation when the comment info is actually not present in `IO` (IIRC, mapping would be called even when the info is not present).


================
Comment at: clang-doc/tool/ClangDocMain.cpp:191
 
-  // Reducing phase
+  auto G = doc::findGeneratorByName(Format);
+  if (!G) {
----------------
If you want to fail early when `Format` is invalid, maybe move it a bit more earlier, e.g. before generating `MapOutput`?


================
Comment at: clang-doc/tool/ClangDocMain.cpp:221
+    }
+    std::error_code FileErr;
+    llvm::raw_fd_ostream InfoOS(InfoPath.get(), FileErr, llvm::sys::fs::F_None);
----------------
nit: I wonder if you could merge creation of `InfoOS` into `getPath` so that the helper function would just return `llvm::Expected<raw_fd_ostream>`


https://reviews.llvm.org/D43667