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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits