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