[PATCH] D119479: [clang][extract-api] Add global record support

[Daniel Grumberg via Phabricator via cfe-commits]

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

You should fix the test to take into account the serializer feedback I left behind



================
Comment at: clang/include/clang/SymbolGraph/API.h:33
+
+struct APIRecord {
+  StringRef Name;
----------------
Might be worth deleting the default constructor.


================
Comment at: clang/include/clang/SymbolGraph/API.h:44
+  /// Discriminator for LLVM-style RTTI (dyn_cast<> et al.)
+  enum RecordKind {
+    RK_Global,
----------------
APIRecord isn't an abstract class so can be instantiated. To be strictly compliant with the LLVM-style RTTI you would need an enum case for a plain record. Otherwise you could introduce a pure virtual function to APIRecord to make abstract. A good candidate for that would be the destructor, if you make that pure virtual but provide an empty implementation, the behaviour should be what you want.


================
Comment at: clang/lib/AST/RawCommentList.cpp:366
 
+  auto DropTrailingNewLines = [](std::string &Str) {
+    while (!Str.empty() && Str.back() == '\n')
----------------
Consider not using a lambda for this since you only do this once. Also consider rewriting the logic as
```
auto LastChar = S.find_last_not_of("\n");
S.erase(LastChar+1, S.size())
```


================
Comment at: clang/lib/AST/RawCommentList.cpp:431
     }
+    std::string Line;
     llvm::StringRef TokText = L.getSpelling(Tok, SourceMgr);
----------------
Might want to use a `SmallString<124>` here as it is likely that the comment line would fit into that and the `+=` operator would only operate on stack storage. In the end of the common case (the line fits in 124 chars) you would only get a single heap allocation when you store emplace it into `Results`.


================
Comment at: clang/lib/SymbolGraph/DeclarationFragments.cpp:54
+  case DeclarationFragments::FragmentKind::GenericParameter:
+    return "genericParam";
+  case DeclarationFragments::FragmentKind::ExternalParam:
----------------
"genericParameter" is what is used in SymbolKit (c.f. https://github.com/apple/swift-docc-symbolkit/blob/main/Sources/SymbolKit/SymbolGraph/Symbol/DeclarationFragments/Fragment/FragmentKind.swift)


================
Comment at: clang/lib/SymbolGraph/DeclarationFragments.cpp:76
+            DeclarationFragments::FragmentKind::TypeIdentifier)
+      .Case("genericParam",
+            DeclarationFragments::FragmentKind::GenericParameter)
----------------
ditto "genericParameter"


================
Comment at: clang/lib/SymbolGraph/ExtractAPIConsumer.cpp:173
+public:
+  explicit ExtractAPIConsumer(ASTContext &Context,
+                              std::unique_ptr<raw_pwrite_stream> OS)
----------------
`explicit` doesn't do anything here since this constructor takes in 2 arguments?


================
Comment at: clang/lib/SymbolGraph/Serialization.cpp:123
+  case Language::ObjC:
+    return "objc";
+
----------------
Should be "occ"


================
Comment at: clang/lib/SymbolGraph/Serialization.cpp:233
+    case GVKind::Function:
+      Kind["identifier"] = (getLanguageName(LangOpts) + ".function").str();
+      Kind["displayName"] = "Function";
----------------
".func"


================
Comment at: clang/lib/SymbolGraph/Serialization.cpp:237
+    case GVKind::Variable:
+      Kind["identifier"] = (getLanguageName(LangOpts) + ".variable").str();
+      Kind["displayName"] = "Variable";
----------------
".variable"


================
Comment at: clang/lib/SymbolGraph/Serialization.cpp:238
+      Kind["identifier"] = (getLanguageName(LangOpts) + ".variable").str();
+      Kind["displayName"] = "Variable";
+      break;
----------------
"Global Variable"


================
Comment at: clang/lib/SymbolGraph/Serialization.cpp:252
+
+const VersionTuple Serializer::FormatVersion{0, 5, 3};
+
----------------
Currently the spec (https://github.com/apple/swift-docc-symbolkit/blob/main/openapi.yaml) lists 0.3.0 as the version. Let's use that.


================
Comment at: clang/lib/SymbolGraph/Serialization.cpp:258
+                  serializeSemanticVersion(FormatVersion));
+  Metadata["generator"] = "clang";
+  return Metadata;
----------------
something more descriptive would be nice here. I think you want the output off `getClangFullRepositoryVersion` or `getClangFullVersion` so we have something meaningful to go off for future bugs.


================
Comment at: clang/lib/SymbolGraph/Serialization.cpp:264
+  Object Module;
+  // FIXME: What to put in here?
+  Module["name"] = "";
----------------
As discussed offline we need to pass clang an extra flag to populate this. I suggest `--product-name`.


================
Comment at: clang/lib/SymbolGraph/Serialization.cpp:283
+  serializeObject(Obj, "docComment", serializeDocComment(Record.Comment));
+  serializeArray(Obj, "declaration",
+                 serializeDeclarationFragments(Record.Declaration));
----------------
This is called "declarationFragments" also this is a function only concept as per the spec https://github.com/apple/swift-docc-symbolkit/blob/224736ddcdfcba87ae92bb5c8d74e8332f218f36/openapi.yaml#L311


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119479