[clang][ExtractAPI] Refactor serializer to the CRTP

[evelez7]

Summary

Refactor SerializerBase and SymbolGraphSerializer to use a visitor pattern described by the CRTP.

[dang]

LGTM with minor changes

clang/include/clang/ExtractAPI/Serialization/SerializerBase.h

58

It would be nice to keep this as default, i.e.

~APISetVisitor() = default;

clang/include/clang/ExtractAPI/Serialization/SymbolGraphSerializer.h

20

In LLVM we tend to explicitly include any header we use and we definitely use the definitions ins API.h

clang/lib/ExtractAPI/Serialization/SymbolGraphSerializer.cpp

748

This isn't part of the visitation scheme but more of a way of serializing a single record. I would prefer to keep this as serializeSingleRecord

808

Does this need to be explicit like this? Would it not work to just call traverseAPISet(); since we would inherit it through inheritance.

[evelez7]

Address review feedback

clang/include/clang/ExtractAPI/Serialization/SerializerBase.h

 //===- ExtractAPI/Serialization/SerializerBase.h ----------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
-/// This file defines the ExtractAPI APISerializer interface.
+/// This file defines the ExtractAPI APISetVisitor interface.
 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_EXTRACTAPI_SERIALIZATION_SERIALIZERBASE_H
 #define LLVM_CLANG_EXTRACTAPI_SERIALIZATION_SERIALIZERBASE_H
 
 #include "clang/ExtractAPI/API.h"
-#include "clang/ExtractAPI/APIIgnoresList.h"
-#include "llvm/Support/raw_ostream.h"
 
 namespace clang {
 namespace extractapi {
 
-/// Common options to customize the serializer output.
-struct APISerializerOption {
-  /// Do not include unnecessary whitespaces to save space.
-  bool Compact;
-};
-
-/// The base interface of serializers for API information.
-class APISerializer {
+/// The base interface of visitors for API information.
+template <typename Derived> class APISetVisitor {
 public:
-  /// Serialize the API information to \p os.
-  virtual void serialize(raw_ostream &os) = 0;
+  void traverseAPISet() {
+    getDerived()->traverseGlobalVariableRecords();
 
-protected:
-  const APISet &API;
+    getDerived()->traverseGlobalFunctionRecords();
 
-  /// The list of symbols to ignore.
-  ///
-  /// Note: This should be consulted before emitting a symbol.
-  const APIIgnoresList &IgnoresList;
+    getDerived()->traverseEnumRecords();
+
+    getDerived()->traverseStructRecords();
+
+    getDerived()->traverseObjCInterfaces();
+
+    getDerived()->traverseObjCProtocols();
+
+    getDerived()->traverseMacroDefinitionRecords();
+
+    getDerived()->traverseTypedefRecords();
+  }
+
+  void traverseGlobalFunctionRecords() {
+    for (const auto &GlobalFunction : API.getGlobalFunctions())
+      getDerived()->visitGlobalFunctionRecord(*GlobalFunction.second);
+  }
+
+  void traverseGlobalVariableRecords() {
+    for (const auto &GlobalVariable : API.getGlobalVariables())
+      getDerived()->visitGlobalVariableRecord(*GlobalVariable.second);
+  }
+
+  void traverseEnumRecords() {
+    for (const auto &Enum : API.getEnums())
+      getDerived()->visitEnumRecord(*Enum.second);
+  }
+
+  void traverseStructRecords() {
+    for (const auto &Struct : API.getStructs())
+      getDerived()->visitStructRecord(*Struct.second);
+  }
 
-  APISerializerOption Options;
+  void traverseObjCInterfaces() {
+    for (const auto &Interface : API.getObjCInterfaces())
+      getDerived()->visitObjCContainerRecord(*Interface.second);
+  }
+
+  void traverseObjCProtocols() {
+    for (const auto &Protocol : API.getObjCProtocols())
+      getDerived()->visitObjCContainerRecord(*Protocol.second);
+  }
+
+  void traverseMacroDefinitionRecords() {
+    for (const auto &Macro : API.getMacros())
+      getDerived()->visitMacroDefinitionRecord(*Macro.second);
+  }
+
+  void traverseTypedefRecords() {
+    for (const auto &Typedef : API.getTypedefs())
+      getDerived()->visitTypedefRecord(*Typedef.second);
+  }
+
+  /// Visit a global function record.
+  void visitGlobalFunctionRecord(const GlobalFunctionRecord &Record){};
+
+  /// Visit a global variable record.
+  void visitGlobalVariableRecord(const GlobalVariableRecord &Record){};
+
+  /// Visit an enum record.
+  void visitEnumRecord(const EnumRecord &Record){};
+
+  /// Visit a struct record.
+  void visitStructRecord(const StructRecord &Record){};
+
+  /// Visit an Objective-C container record.
+  void visitObjCContainerRecord(const ObjCContainerRecord &Record){};
+
+  /// Visit a macro definition record.
+  void visitMacroDefinitionRecord(const MacroDefinitionRecord &Record){};
+
+  /// Visit a typedef record.
+  void visitTypedefRecord(const TypedefRecord &Record){};
+
+protected:
+  const APISet &API;
 
 public:
-  APISerializer() = delete;
-  APISerializer(const APISerializer &) = delete;
-  APISerializer(APISerializer &&) = delete;
-  APISerializer &operator=(const APISerializer &) = delete;
-  APISerializer &operator=(APISerializer &&) = delete;
+  APISetVisitor() = delete;
+  APISetVisitor(const APISetVisitor &) = delete;
+  APISetVisitor(APISetVisitor &&) = delete;
+  APISetVisitor &operator=(const APISetVisitor &) = delete;
+  APISetVisitor &operator=(APISetVisitor &&) = delete;
 
 protected:
-  APISerializer(const APISet &API, const APIIgnoresList &IgnoresList,
-                APISerializerOption Options = {})
-      : API(API), IgnoresList(IgnoresList), Options(Options) {}
+  APISetVisitor(const APISet &API) : API(API) {}
+  ~APISetVisitor() = default;
 
-  virtual ~APISerializer() = default;
+  Derived *getDerived() { return static_cast<Derived *>(this); };
 };
 
 } // namespace extractapi
 } // namespace clang
 
 #endif // LLVM_CLANG_EXTRACTAPI_SERIALIZATION_SERIALIZERBASE_H

clang/include/clang/ExtractAPI/Serialization/SymbolGraphSerializer.h

 //===- ExtractAPI/Serialization/SymbolGraphSerializer.h ---------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
 /// This file defines the SymbolGraphSerializer class.
 ///
-/// Implement an APISerializer for the Symbol Graph format for ExtractAPI.
-/// See https://github.com/apple/swift-docc-symbolkit.
+/// Implement an APISetVisitor to serialize the APISet into the Symbol Graph
+/// format for ExtractAPI. See https://github.com/apple/swift-docc-symbolkit.
 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_EXTRACTAPI_SERIALIZATION_SYMBOLGRAPHSERIALIZER_H
 #define LLVM_CLANG_EXTRACTAPI_SERIALIZATION_SYMBOLGRAPHSERIALIZER_H
 
 #include "clang/ExtractAPI/API.h"
 #include "clang/ExtractAPI/APIIgnoresList.h"
 #include "clang/ExtractAPI/Serialization/SerializerBase.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/VersionTuple.h"
 #include "llvm/Support/raw_ostream.h"
 #include <optional>
 
 namespace clang {
 namespace extractapi {
 
 using namespace llvm::json;
 
-/// The serializer that organizes API information in the Symbol Graph format.
+/// Common options to customize the visitor output.
+struct SymbolGraphSerializerOption {
+  /// Do not include unnecessary whitespaces to save space.
+  bool Compact;
+};
+
+/// The visitor that organizes API information in the Symbol Graph format.
 ///
 /// The Symbol Graph format (https://github.com/apple/swift-docc-symbolkit)
 /// models an API set as a directed graph, where nodes are symbol declarations,
 /// and edges are relationships between the connected symbols.
-class SymbolGraphSerializer : public APISerializer {
-  virtual void anchor();
-
+class SymbolGraphSerializer : public APISetVisitor<SymbolGraphSerializer> {
   /// A JSON array of formatted symbols in \c APISet.
   Array Symbols;
 
   /// A JSON array of formatted symbol relationships in \c APISet.
   Array Relationships;
 
   /// The Symbol Graph format version used by this serializer.
   static const VersionTuple FormatVersion;
 
-  /// Indicates whether child symbols should be serialized. This is mainly
+  /// Indicates whether child symbols should be visited. This is mainly
   /// useful for \c serializeSingleSymbolSGF.
   bool ShouldRecurse;
 
 public:
   /// Serialize the APIs in \c APISet in the Symbol Graph format.
   ///
   /// \returns a JSON object that contains the root of the formatted
   /// Symbol Graph.
   Object serialize();
 
-  /// Implement the APISerializer::serialize interface. Wrap serialize(void) and
-  /// write out the serialized JSON object to \p os.
-  void serialize(raw_ostream &os) override;
+  ///  Wrap serialize(void) and write out the serialized JSON object to \p os.
+  void serialize(raw_ostream &os);
 
   /// Serialize a single symbol SGF. This is primarily used for libclang.
   ///
   /// \returns an optional JSON Object representing the payload that libclang
   /// expects for providing symbol information for a single symbol. If this is
   /// not a known symbol returns \c std::nullopt.
   static std::optional<Object> serializeSingleSymbolSGF(StringRef USR,
                                                         const APISet &API);
private:
 
   /// Serialize the \p Kind relationship between \p Source and \p Target.
   ///
   /// Record the relationship between the two symbols in
   /// SymbolGraphSerializer::Relationships.
   void serializeRelationship(RelationshipKind Kind, SymbolReference Source,
                              SymbolReference Target);
 
-  /// Serialize a global function record.
-  void serializeGlobalFunctionRecord(const GlobalFunctionRecord &Record);
+protected:
+  /// The list of symbols to ignore.
+  ///
+  /// Note: This should be consulted before emitting a symbol.
+  const APIIgnoresList &IgnoresList;
+
+  SymbolGraphSerializerOption Options;
+
+public:
+  /// Visit a global function record.
+  void visitGlobalFunctionRecord(const GlobalFunctionRecord &Record);
 
-  /// Serialize a global variable record.
-  void serializeGlobalVariableRecord(const GlobalVariableRecord &Record);
+  /// Visit a global variable record.
+  void visitGlobalVariableRecord(const GlobalVariableRecord &Record);
 
-  /// Serialize an enum record.
-  void serializeEnumRecord(const EnumRecord &Record);
+  /// Visit an enum record.
+  void visitEnumRecord(const EnumRecord &Record);
 
-  /// Serialize a struct record.
-  void serializeStructRecord(const StructRecord &Record);
+  /// Visit a struct record.
+  void visitStructRecord(const StructRecord &Record);
 
-  /// Serialize an Objective-C container record.
-  void serializeObjCContainerRecord(const ObjCContainerRecord &Record);
+  /// Visit an Objective-C container record.
+  void visitObjCContainerRecord(const ObjCContainerRecord &Record);
 
-  /// Serialize a macro definition record.
-  void serializeMacroDefinitionRecord(const MacroDefinitionRecord &Record);
+  /// Visit a macro definition record.
+  void visitMacroDefinitionRecord(const MacroDefinitionRecord &Record);
 
-  /// Serialize a typedef record.
-  void serializeTypedefRecord(const TypedefRecord &Record);
+  /// Visit a typedef record.
+  void visitTypedefRecord(const TypedefRecord &Record);
 
+  /// Serialize a single record.
   void serializeSingleRecord(const APIRecord *Record);
 
-public:
   SymbolGraphSerializer(const APISet &API, const APIIgnoresList &IgnoresList,
-                        APISerializerOption Options = {},
+                        SymbolGraphSerializerOption Options = {},
                         bool ShouldRecurse = true)
-      : APISerializer(API, IgnoresList, Options), ShouldRecurse(ShouldRecurse) {
-  }
+      : APISetVisitor(API), ShouldRecurse(ShouldRecurse),
+        IgnoresList(IgnoresList), Options(Options) {}
 };
 
 } // namespace extractapi
 } // namespace clang
 
 #endif // LLVM_CLANG_EXTRACTAPI_SERIALIZATION_SYMBOLGRAPHSERIALIZER_H

clang/lib/ExtractAPI/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   TargetParser
   )
 
 add_clang_library(clangExtractAPI
   API.cpp
   APIIgnoresList.cpp
   AvailabilityInfo.cpp
   ExtractAPIConsumer.cpp
   DeclarationFragments.cpp
-  Serialization/SerializerBase.cpp
   Serialization/SymbolGraphSerializer.cpp
   TypedefUnderlyingTypeResolver.cpp
 
   LINK_LIBS
   clangAST
   clangBasic
   clangFrontend
   clangIndex
   clangLex
   )

clang/lib/ExtractAPI/Serialization/SerializerBase.cpp

-//===- ExtractAPI/Serialization/SerializerBase.cpp --------------*- C++ -*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-///
-/// \file
-/// This file implements the APISerializer interface.
-///
-//===----------------------------------------------------------------------===//
-
-#include "clang/ExtractAPI/Serialization/SerializerBase.h"
-#include "llvm/Support/raw_ostream.h"
-
-using namespace clang::extractapi;
-
-void APISerializer::serialize(llvm::raw_ostream &os) {}

clang/lib/ExtractAPI/Serialization/SymbolGraphSerializer.cpp

 //===- ExtractAPI/Serialization/SymbolGraphSerializer.cpp -------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
 /// This file implements the SymbolGraphSerializer.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "clang/ExtractAPI/Serialization/SymbolGraphSerializer.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/Version.h"
-#include "clang/ExtractAPI/API.h"
-#include "clang/ExtractAPI/APIIgnoresList.h"
 #include "clang/ExtractAPI/DeclarationFragments.h"
-#include "clang/ExtractAPI/Serialization/SerializerBase.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/STLFunctionalExtras.h"
-#include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Compiler.h"
-#include "llvm/Support/JSON.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/VersionTuple.h"
 #include <optional>
 #include <type_traits>
 
 using namespace clang;
 using namespace clang::extractapi;
 using namespace llvm;
Object serializeParentContext(const PathComponent &PC, Language Lang) {
   ParentContextElem["kind"] = serializeSymbolKind(PC.Kind, Lang)["identifier"];
   return ParentContextElem;
 }
 
 template <typename RecordTy>
 Array generateParentContexts(const RecordTy &Record, const APISet &API,
                              Language Lang) {
   Array ParentContexts;
-  generatePathComponents(Record, API,
-                         [Lang, &ParentContexts](const PathComponent &PC) {
-                           ParentContexts.push_back(
-                               serializeParentContext(PC, Lang));
-                         });
+  generatePathComponents(
+      Record, API, [Lang, &ParentContexts](const PathComponent &PC) {
+        ParentContexts.push_back(serializeParentContext(PC, Lang));
 
   return ParentContexts;
 }
 
 } // namespace
 
-void SymbolGraphSerializer::anchor() {}
-
 /// Defines the format version emitted by SymbolGraphSerializer.
 const VersionTuple SymbolGraphSerializer::FormatVersion{0, 5, 3};
 
 Object SymbolGraphSerializer::serializeMetadata() const {
   Object Metadata;
   serializeObject(Metadata, "formatVersion",
                   serializeSemanticVersion(FormatVersion));
   Metadata["generator"] = clang::getClangFullVersion();
void SymbolGraphSerializer::serializeRelationship(RelationshipKind Kind,
   Relationship["source"] = Source.USR;
   Relationship["target"] = Target.USR;
   Relationship["targetFallback"] = Target.Name;
   Relationship["kind"] = getRelationshipString(Kind);
 
   Relationships.emplace_back(std::move(Relationship));
 }
 
-void SymbolGraphSerializer::serializeGlobalFunctionRecord(
+void SymbolGraphSerializer::visitGlobalFunctionRecord(
     const GlobalFunctionRecord &Record) {
   auto Obj = serializeAPIRecord(Record);
   if (!Obj)
     return;
 
   Symbols.emplace_back(std::move(*Obj));
 }
 
-void SymbolGraphSerializer::serializeGlobalVariableRecord(
+void SymbolGraphSerializer::visitGlobalVariableRecord(
     const GlobalVariableRecord &Record) {
   auto Obj = serializeAPIRecord(Record);
   if (!Obj)
     return;
 
   Symbols.emplace_back(std::move(*Obj));
 }
 
-void SymbolGraphSerializer::serializeEnumRecord(const EnumRecord &Record) {
+void SymbolGraphSerializer::visitEnumRecord(const EnumRecord &Record) {
   auto Enum = serializeAPIRecord(Record);
   if (!Enum)
     return;
 
   Symbols.emplace_back(std::move(*Enum));
   serializeMembers(Record, Record.Constants);
 }
 
-void SymbolGraphSerializer::serializeStructRecord(const StructRecord &Record) {
+void SymbolGraphSerializer::visitStructRecord(const StructRecord &Record) {
   auto Struct = serializeAPIRecord(Record);
   if (!Struct)
     return;
 
   Symbols.emplace_back(std::move(*Struct));
   serializeMembers(Record, Record.Fields);
 }
 
-void SymbolGraphSerializer::serializeObjCContainerRecord(
+void SymbolGraphSerializer::visitObjCContainerRecord(
     const ObjCContainerRecord &Record) {
   auto ObjCContainer = serializeAPIRecord(Record);
   if (!ObjCContainer)
     return;
 
   Symbols.emplace_back(std::move(*ObjCContainer));
 
   serializeMembers(Record, Record.Ivars);
for (const auto *Category : ObjCInterface->Categories) {
 
       // Surface the protocols of the category to the interface.
       for (const auto &Protocol : Category->Protocols)
         serializeRelationship(RelationshipKind::ConformsTo, Record, Protocol);
     }
   }
 }
 
-void SymbolGraphSerializer::serializeMacroDefinitionRecord(
+void SymbolGraphSerializer::visitMacroDefinitionRecord(
     const MacroDefinitionRecord &Record) {
   auto Macro = serializeAPIRecord(Record);
 
   if (!Macro)
     return;
 
   Symbols.emplace_back(std::move(*Macro));
 }
 
 void SymbolGraphSerializer::serializeSingleRecord(const APIRecord *Record) {
   switch (Record->getKind()) {
   case APIRecord::RK_Unknown:
     llvm_unreachable("Records should have a known kind!");
   case APIRecord::RK_GlobalFunction:
-    serializeGlobalFunctionRecord(*cast<GlobalFunctionRecord>(Record));
+    visitGlobalFunctionRecord(*cast<GlobalFunctionRecord>(Record));
     break;
   case APIRecord::RK_GlobalVariable:
-    serializeGlobalVariableRecord(*cast<GlobalVariableRecord>(Record));
+    visitGlobalVariableRecord(*cast<GlobalVariableRecord>(Record));
     break;
   case APIRecord::RK_Enum:
-    serializeEnumRecord(*cast<EnumRecord>(Record));
+    visitEnumRecord(*cast<EnumRecord>(Record));
     break;
   case APIRecord::RK_Struct:
-    serializeStructRecord(*cast<StructRecord>(Record));
+    visitStructRecord(*cast<StructRecord>(Record));
     break;
   case APIRecord::RK_ObjCInterface:
-    serializeObjCContainerRecord(*cast<ObjCInterfaceRecord>(Record));
+    visitObjCContainerRecord(*cast<ObjCInterfaceRecord>(Record));
     break;
   case APIRecord::RK_ObjCProtocol:
-    serializeObjCContainerRecord(*cast<ObjCProtocolRecord>(Record));
+    visitObjCContainerRecord(*cast<ObjCProtocolRecord>(Record));
     break;
   case APIRecord::RK_MacroDefinition:
-    serializeMacroDefinitionRecord(*cast<MacroDefinitionRecord>(Record));
+    visitMacroDefinitionRecord(*cast<MacroDefinitionRecord>(Record));
     break;
   case APIRecord::RK_Typedef:
-    serializeTypedefRecord(*cast<TypedefRecord>(Record));
+    visitTypedefRecord(*cast<TypedefRecord>(Record));
     break;
   default:
     if (auto Obj = serializeAPIRecord(*Record)) {
       Symbols.emplace_back(std::move(*Obj));
       auto &ParentInformation = Record->ParentInformation;
       if (!ParentInformation.empty())
         serializeRelationship(RelationshipKind::MemberOf, *Record,
                               *ParentInformation.ParentRecord);
     }
     break;
   }
 }
 
-void SymbolGraphSerializer::serializeTypedefRecord(
-    const TypedefRecord &Record) {
+void SymbolGraphSerializer::visitTypedefRecord(const TypedefRecord &Record) {
   // Typedefs of anonymous types have their entries unified with the underlying
   // type.
   bool ShouldDrop = Record.UnderlyingType.Name.empty();
   // enums declared with `NS_OPTION` have a named enum and a named typedef, with
   // the same name
   ShouldDrop |= (Record.UnderlyingType.Name == Record.Name);
   if (ShouldDrop)
     return;
 
   auto Typedef = serializeAPIRecord(Record);
   if (!Typedef)
     return;
 
   (*Typedef)["type"] = Record.UnderlyingType.USR;
 
   Symbols.emplace_back(std::move(*Typedef));
 }
 
 Object SymbolGraphSerializer::serialize() {
-  // Serialize global variables in the API set.
-  for (const auto &GlobalVar : API.getGlobalVariables())
-    serializeGlobalVariableRecord(*GlobalVar.second);
-
-  for (const auto &GlobalFunction : API.getGlobalFunctions())
-    serializeGlobalFunctionRecord(*GlobalFunction.second);
-
-  // Serialize enum records in the API set.
-  for (const auto &Enum : API.getEnums())
-    serializeEnumRecord(*Enum.second);
-
-  // Serialize struct records in the API set.
-  for (const auto &Struct : API.getStructs())
-    serializeStructRecord(*Struct.second);
-
-  // Serialize Objective-C interface records in the API set.
-  for (const auto &ObjCInterface : API.getObjCInterfaces())
-    serializeObjCContainerRecord(*ObjCInterface.second);
-
-  // Serialize Objective-C protocol records in the API set.
-  for (const auto &ObjCProtocol : API.getObjCProtocols())
-    serializeObjCContainerRecord(*ObjCProtocol.second);
-
-  for (const auto &Macro : API.getMacros())
-    serializeMacroDefinitionRecord(*Macro.second);
-
-  for (const auto &Typedef : API.getTypedefs())
-    serializeTypedefRecord(*Typedef.second);
-
+  traverseAPISet();
   return serializeCurrentGraph();
 }
 
 Object SymbolGraphSerializer::serializeCurrentGraph() {
   Object Root;
   serializeObject(Root, "metadata", serializeMetadata());
   serializeObject(Root, "module", serializeModule());