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

[Daniel Grumberg via Phabricator via cfe-commits]

dang added a comment.

LGTM with minor changes



================
Comment at: clang/include/clang/ExtractAPI/Serialization/SerializerBase.h:58
 
-  virtual ~APISerializer() = default;
 };
----------------
It would be nice to keep this as default, i.e. 
```
~APISetVisitor() = default;
```


================
Comment at: clang/include/clang/ExtractAPI/Serialization/SymbolGraphSerializer.h:20
 
-#include "clang/ExtractAPI/API.h"
 #include "clang/ExtractAPI/APIIgnoresList.h"
----------------
In LLVM we tend to explicitly include any header we use and we definitely use the definitions ins API.h


================
Comment at: clang/lib/ExtractAPI/Serialization/SymbolGraphSerializer.cpp:748
 
-void SymbolGraphSerializer::serializeSingleRecord(const APIRecord *Record) {
+void SymbolGraphSerializer::visitSingleRecord(const APIRecord *Record) {
   switch (Record->getKind()) {
----------------
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` 


================
Comment at: clang/lib/ExtractAPI/Serialization/SymbolGraphSerializer.cpp:808
 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);
-
+  APISetVisitor::traverseAPISet();
   return serializeCurrentGraph();
----------------
Does this need to be explicit like this? Would it not work to just call `traverseAPISet();` since we would inherit it through inheritance.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151477