[PATCH] D139115: [clang][ExtractAPI] Add support for single symbol SGF

[Zixu Wang via Phabricator via cfe-commits]

zixuw added inline comments.


================
Comment at: clang/include/clang/ExtractAPI/API.h:128-130
+  /// Store hierarchy information for a given record.
+  ///
+  /// This is roughly analogous to the DeclContext hierarchy for an AST Node.
----------------
Misplaced comment?


================
Comment at: clang/include/clang/ExtractAPI/API.h:234
     ReadOnly = 1,
-    Class = 1 << 1,
     Dynamic = 1 << 2,
----------------
What's the reason for refactoring out instance vs. class property? Looks like this should be a separated patch


================
Comment at: clang/include/clang/ExtractAPI/API.h:301
 
   static bool classof(const APIRecord *Record) {
+    return Record->getKind() == RK_ObjCInstanceProperty ||
----------------
No need for `classof` in an abstract node


================
Comment at: clang/include/clang/ExtractAPI/API.h:393
 
   static bool classof(const APIRecord *Record) {
+    return Record->getKind() == RK_ObjCClassMethod ||
----------------
No need for `classof` in an abstract node


================
Comment at: clang/include/clang/ExtractAPI/API.h:401
+
+struct ObjCInstanceMethodRecord : ObjCMethodRecord {
+  ObjCInstanceMethodRecord(StringRef USR, StringRef Name, PresumedLoc Loc,
----------------
Same question as instance/class property


================
Comment at: clang/include/clang/ExtractAPI/API.h:478
   virtual ~ObjCContainerRecord() = 0;
+
+  static bool classof(const APIRecord *Record) {
----------------
No need for `classof` in an abstract node


================
Comment at: clang/include/clang/ExtractAPI/DeclarationFragments.h:91
+    /// The associated declaration, if applicable.
+    const Decl *Declaration;
+
----------------
Is the decl guaranteed to be available when `Fragment` is used?
In our existing use case this might be fine as the serializer will be used while the AST is still alive. But this is actually the first thing that adds the dependency to the AST IIRC. So this makes the whole APISet dependent on the AST


================
Comment at: clang/lib/ExtractAPI/API.cpp:145
+APISet::addObjCInterface(StringRef Name, StringRef USR, PresumedLoc Loc,
+                         AvailabilitySet Availabilities, LinkageInfo Linkage,
+                         const DocComment &Comment,
----------------
nit: tab


================
Comment at: clang/lib/ExtractAPI/API.cpp:169
+    Record = std::make_unique<ObjCClassMethodRecord>(
+        USR, Name, Loc, std::move(Availabilities), Comment, Declaration,
+        SubHeading, Signature, IsFromSystemHeader);
----------------
nit: tab


================
Comment at: clang/lib/ExtractAPI/API.cpp:193
+    Record = std::make_unique<ObjCClassPropertyRecord>(
+        USR, Name, Loc, std::move(Availabilities), Comment, Declaration,
+        SubHeading, Attributes, GetterName, SetterName, IsOptional,
----------------
nit: tab


================
Comment at: clang/tools/c-index-test/c-index-test.c:4927-4928
+  fprintf(stderr,
+          "       c-index-test -write-pch <file> <compiler arguments>\n"
+          "       c-index-test -compilation-db [lookup <filename>] database\n");
   fprintf(stderr,
----------------
nit: tab


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139115