[PATCH] D151293: [clang][ExtractAPI] Refactor serializer to the CRTP

[Daniel Grumberg via Phabricator via cfe-commits]

dang requested changes to this revision.
dang added a comment.
This revision now requires changes to proceed.

Great start but there are still some rough edges to polish!



================
Comment at: clang/include/clang/ExtractAPI/Serialization/SerializerBase.h:19
 #include "clang/ExtractAPI/APIIgnoresList.h"
+#include "llvm/Support/JSON.h"
 #include "llvm/Support/raw_ostream.h"
----------------
This shouldn't be needed at this point.


================
Comment at: clang/include/clang/ExtractAPI/Serialization/SerializerBase.h:28-31
+struct APISetVisitorOption {
   /// Do not include unnecessary whitespaces to save space.
   bool Compact;
 };
----------------
This is very specific to SymbolGraphSerializer or at last to JSON.


================
Comment at: clang/include/clang/ExtractAPI/Serialization/SerializerBase.h:37
+  /// Traverse the API information to output to \p os.
+  void traverseAPISet(raw_ostream &os){};
+
----------------
This shouldn't be needed anymore.


================
Comment at: clang/include/clang/ExtractAPI/Serialization/SerializerBase.h:84
+    for (const auto &Protocol : API.getObjCProtocols())
+      getDerived()->visitObjCContainerRecord(*Protocol.second);
+  }
----------------
This should visit some sort Protocol Records specifically. Likewise for interfaces.


================
Comment at: clang/include/clang/ExtractAPI/Serialization/SerializerBase.h:100
+  /// Visit a global variable record.
+  void visitGlobalVariableRecord(const GlobalVariableRecord &Record);
+
----------------
This should have a default implementation.


================
Comment at: clang/include/clang/ExtractAPI/Serialization/SerializerBase.h:138
 protected:
-  APISerializer(const APISet &API, const APIIgnoresList &IgnoresList,
-                APISerializerOption Options = {})
+  APISetVisitor(const APISet &API, const APIIgnoresList &IgnoresList,
+                APISetVisitorOption Options = {})
----------------
I think that `APIIgnoresList` Should be pushed down into the SymbolGraphSerializer.


================
Comment at: clang/include/clang/ExtractAPI/Serialization/SerializerBase.h:142
 
-  virtual ~APISerializer() = default;
+  virtual ~APISetVisitor() = default;
+  Derived *getDerived() { return static_cast<Derived *>(this); };
----------------
Don't need a virtual destructor anymore.


================
Comment at: clang/include/clang/ExtractAPI/Serialization/SymbolGraphSerializer.h:12
 ///
-/// Implement an APISerializer for the Symbol Graph format for ExtractAPI.
+/// Implement an APISetVisitor for the Symbol Graph format for ExtractAPI.
 /// See https://github.com/apple/swift-docc-symbolkit.
----------------



================
Comment at: clang/include/clang/ExtractAPI/Serialization/SymbolGraphSerializer.h:94
   /// Just serialize the currently recorded objects in Symbol Graph format.
-  Object serializeCurrentGraph();
+  Object visitCurrentGraph();
 
----------------
I think these should still be called serializeXXX as they aren't part of the visitation interface and are very specific to the symbol graph format.


================
Comment at: clang/include/clang/ExtractAPI/Serialization/SymbolGraphSerializer.h:129
   template <typename MemberTy>
-  void serializeMembers(const APIRecord &Record,
-                        const SmallVector<std::unique_ptr<MemberTy>> &Members);
+  void visitMembers(const APIRecord &Record,
+                    const SmallVector<std::unique_ptr<MemberTy>> &Members);
----------------
I think we shouldn't need to have this method. APISetVisitor should handle calling the relevant visitMethods for the members.


================
Comment at: clang/lib/ExtractAPI/Serialization/SymbolGraphSerializer.cpp:221
 /// references, and the interface language name.
-Object serializeIdentifier(const APIRecord &Record, Language Lang) {
+Object visitIdentifier(const APIRecord &Record, Language Lang) {
   Object Identifier;
----------------
I think all these helper methods for serializing specific chunks of symbol graphs should still be names serialize


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151293/new/

https://reviews.llvm.org/D151293