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

[Daniel Grumberg via Phabricator via cfe-commits]

dang added inline comments.


================
Comment at: clang/include/clang/ExtractAPI/API.h:34
 
+/// Documentation comment is a vector of RawComment::CommentLine.
+///
----------------
You should use the actual name of the type or "This"


================
Comment at: clang/include/clang/ExtractAPI/API.h:37
+/// Each line represents one line of striped documentation comment,
+/// with source range information. This could simplify calculating the source
+/// location of a character in the doc comment for pointing back to the source
----------------
If it wouldn't make things simpler we wouldn't have it. IMHO it is better to use definitive terms in documentation. here it would "This simplifies calculating [...]"


================
Comment at: clang/include/clang/ExtractAPI/API.h:103
 
+/// GlobalRecord holds information of a global variable or a function.
 struct GlobalRecord : APIRecord {
----------------
Nit: instead of "of a" maybe use "associated with"


================
Comment at: clang/include/clang/ExtractAPI/API.h:123
+private:
+  virtual void anchor();
 };
----------------
This is new so I don't think it belongs in this patch.



================
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,
----------------
why is stuff reordered in the patch?


================
Comment at: clang/include/clang/ExtractAPI/DeclarationFragments.h:108
+  ///
+  /// A helper function to quicking move the fragments in another
+  /// DeclarationFragments and concatenate.
----------------
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". 


================
Comment at: clang/include/clang/ExtractAPI/Serialization/SerializerBase.h:33
+/// The base interface of JSON serializers for API information.
+class APIJSONSerializer {
+public:
----------------
If we are going for a base class, I am not sure that the base should be JSON specific.


================
Comment at: clang/include/clang/ExtractAPI/Serialization/SymbolGraphSerializer.h:34
+class SymbolGraphSerializer : public APIJSONSerializer {
+  virtual void anchor();
+
----------------
Not sure why we need this.


================
Comment at: clang/lib/ExtractAPI/API.cpp:34
   if (Result.second) {
+    // Create the record if not already exist.
     auto Record = APIRecordUniquePtr<GlobalRecord>(new (Allocator) GlobalRecord{
----------------
"Create the record if it does not already exist."


================
Comment at: clang/lib/ExtractAPI/ExtractAPIConsumer.cpp:195
+    // the Symbol Graph format.
+    SymbolGraphSerializer SGSerializer(Visitor.getAPI());
+    SGSerializer.serialize(*OS);
----------------
The kind of serializer should be made configurable somehow.


================
Comment at: clang/lib/ExtractAPI/Serialization/SymbolGraphSerializer.cpp:26
 
-namespace {
+namespace { // Helper functions.
 
----------------
This is unnecessary imho.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122160