[PATCH] D122160: [clang][extract-api] Refactor ExtractAPI and improve docs

[Zixu Wang via Phabricator via cfe-commits]

zixuw added inline comments.


================
Comment at: clang/include/clang/ExtractAPI/API.h:123
+private:
+  virtual void anchor();
 };
----------------
dang wrote:
> This is new so I don't think it belongs in this patch.
> 
This is to follow the LLVM Coding Standards guideline of providing an out-of-line virtual method anchor for a class defined in a header file and has a vtable. Still coding style/standard fix/improvement.


================
Comment at: clang/include/clang/ExtractAPI/API.h:129
 public:
-  APISet(const llvm::Triple &Target, const LangOptions &LangOpts)
-      : Target(Target), LangOpts(LangOpts) {}
-
-  const llvm::Triple &getTarget() const { return Target; }
-  const LangOptions &getLangOpts() const { return LangOpts; }
-
+  /// Create and add a GlobalRecord of kind \p Kind into the API set.
   GlobalRecord *addGlobal(GVKind Kind, StringRef Name, StringRef USR,
----------------
dang wrote:
> why is stuff reordered in the patch?
I looked into some other class definitions inside Clang, and was trying to list "interesting" APIs upfront and less-interesting stuff like constructors near the end for large class definitions to make them more readable.


================
Comment at: clang/include/clang/ExtractAPI/DeclarationFragments.h:108
+  ///
+  /// A helper function to quicking move the fragments in another
+  /// DeclarationFragments and concatenate.
----------------
dang wrote:
> Not sure an explanation of the process is needed. Anyway "quicking" is not a word ;) I would suggest "Other is moved from and can not be used after a call to this method". 
Whops didn't realize that I typed that. Friday's work huh? :p


================
Comment at: clang/include/clang/ExtractAPI/Serialization/SymbolGraphSerializer.h:34
+class SymbolGraphSerializer : public APIJSONSerializer {
+  virtual void anchor();
+
----------------
dang wrote:
> Not sure why we need this.
As above. Refactor to follow LLVM Coding Standards.


================
Comment at: clang/lib/ExtractAPI/ExtractAPIConsumer.cpp:195
+    // the Symbol Graph format.
+    SymbolGraphSerializer SGSerializer(Visitor.getAPI());
+    SGSerializer.serialize(*OS);
----------------
dang wrote:
> The kind of serializer should be made configurable somehow.
Yes for sure. I didn't include that change here because it would probably involve adding command line options and changing the driver, which should have its own patch later.
I added a FIXME here to point it out for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122160