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

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: clang-doc/Representation.h:238
 
+  // Non-const accessors for YAML output.
+  std::vector<NamespaceInfo> &getNamespaceInfos() { return NamespaceInfos; }
----------------
There are two alternatives to avoid this:
1) Pull the info storage into a separate struct e.g. `struct InfoSet::Infos`, and add a constructor setter `setInfos(Infos &&)`;
2) `friend yaml::MappingTraits<InfoSet>;`


================
Comment at: clang-doc/generators/CMakeLists.txt:3
+
+add_clang_library(clangDocGenerators
+  YAMLGenerator.cpp
----------------
Why is this a separate library?


================
Comment at: clang-doc/generators/Generators.h:24
+
+class Generator {
+public:
----------------
Please add documentation.


================
Comment at: clang-doc/generators/Generators.h:26
+public:
+  Generator(std::unique_ptr<InfoSet> &IS, llvm::StringRef Root,
+            llvm::StringRef Format)
----------------
The parameters are not trivial. Could you add documentation?


================
Comment at: clang-doc/generators/Generators.h:26
+public:
+  Generator(std::unique_ptr<InfoSet> &IS, llvm::StringRef Root,
+            llvm::StringRef Format)
----------------
ioeric wrote:
> The parameters are not trivial. Could you add documentation?
Passing a reference to a unique pointer seems a bit strange. If `Generator` doesn't own IS, it should be passed by reference; otherwise, this should be passed by value.


================
Comment at: clang-doc/generators/Generators.h:28
+            llvm::StringRef Format)
+      : IS(IS), Root(Root), Format(Format) {}
+  virtual ~Generator() = default;
----------------
Have you considered passing a output stream instead of passing in a directory and writing output to a hardcoded name? And I think `Root` (or a output stream) should be passed in via `generate`instead the constructor. 


================
Comment at: clang-doc/generators/Generators.h:39
+
+class YAMLGenerator : public Generator {
+public:
----------------
Please add documentation.


================
Comment at: clang-doc/generators/Generators.h:49
+
+class GeneratorFactory {
+public:
----------------
Please add documentation and explain why this is needed. 


================
Comment at: clang-doc/generators/Generators.h:49
+
+class GeneratorFactory {
+public:
----------------
ioeric wrote:
> Please add documentation and explain why this is needed. 
If you plan to plugin in more generators (e.g. in your customized build of clang-doc), consider using `llvm::Registry` which allows you to link in new generators without having to modify code. See `clang::clangd::URISchemeRegistry` for sample usage.


================
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) {
----------------
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. 


================
Comment at: clang-doc/generators/YAMLGenerator.cpp:250
+  std::error_code OutErrorInfo;
+  llvm::raw_fd_ostream OS(Path, OutErrorInfo, llvm::sys::fs::F_None);
+  if (OutErrorInfo != OK) {
----------------
Note that this would only work on real file system. You would not be able to run this in unit tests, and some tool executors run on virtual file system as well, so if you do go with directory, you would also want to pass in a VFS (`clang/Basic/VirtualFileSystem.h`).


https://reviews.llvm.org/D43667