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

[Julie Hockett via Phabricator via cfe-commits]

juliehockett added inline comments.


================
Comment at: clang-doc/YAMLGenerator.cpp:265
+// and thus register the generator.
+volatile int YAMLGeneratorAnchorSource = 0;
+
----------------
ioeric wrote:
> nit: add `static`?
`static` declares it as file-local, and so the `extern` reference to this in Generators.cpp wouldn't be allowed.


================
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) {
----------------
ioeric wrote:
> 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).
Sorry this slipped through before -- it's a pointer because it has to be able to store itself (the other infos just store the `CommentInfo` directly, but each `CommentInfo` can have comment children). The pointer only appears there. 


================
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);
----------------
ioeric wrote:
> 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>`
You can't, because when you return `llvm::make_error` it invokes a copy constructor, which is deleted in `raw_ostream`s.


https://reviews.llvm.org/D43667