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

[Erick Velez via Phabricator via cfe-commits]

evelez7 abandoned this revision.
evelez7 added inline comments.


================
Comment at: clang/include/clang/ExtractAPI/Serialization/SerializerBase.h:28-31
+struct APISetVisitorOption {
   /// Do not include unnecessary whitespaces to save space.
   bool Compact;
 };
----------------
dang wrote:
> This is very specific to SymbolGraphSerializer or at last to JSON.
If there aren't any base visitor options, I'll just move this to `SymbolGraphSerializer` without any inheritance.


================
Comment at: clang/include/clang/ExtractAPI/Serialization/SerializerBase.h:84
+    for (const auto &Protocol : API.getObjCProtocols())
+      getDerived()->visitObjCContainerRecord(*Protocol.second);
+  }
----------------
dang wrote:
> This should visit some sort Protocol Records specifically. Likewise for interfaces.
Not sure I understand, these functions essentially replace the loops in the old `SymbolGraphSerializer::serialize` loops, which are now grouped as function calls in `APISetVisitor::traverseAPISet`. Should I add functions that take Protocols, Interfaces as parameters to visit them specifically?


================
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);
----------------
dang wrote:
> I think we shouldn't need to have this method. APISetVisitor should handle calling the relevant visitMethods for the members.
I've been considering how to move it to `APISetVisitor`. Its current implementation doesn't seem like it would fit well with a generic base implementation. It would be overridden to call `visitRelationship` which relies on a `RelationshipKind` parameter. Since that's SymbolKit specific, I don't think we'd raise that to the base class.

If there was a base relationship struct that `RelationshipKind` inherited from, it might solve that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151293