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

[Daniel Grumberg via Phabricator via cfe-commits]

dang accepted this revision.
dang added a comment.
This revision is now accepted and ready to land.

LGTM once the last few bits of feedback I left are considered.



================
Comment at: clang/include/clang/ExtractAPI/API.h:139
 
+  /// Create and add a global variable record into the API set.
   GlobalRecord *addGlobalVar(StringRef Name, StringRef USR, PresumedLoc Loc,
----------------
Maybe we should document that the caller is meant to keep `Name` and `USR` alive. Name is expected to refer to the appropriate `Decl` member so it shouldn't be an issue, but this usage should be documented. WRT `USR` these comments would be a good place to mention the existence of `recordUSR` and the `copyString` method as a way of getting USR strings that are guaranteed to have the right lifetime.


================
Comment at: clang/include/clang/ExtractAPI/API.h:194
+  /// \returns a StringRef of the copied string in the \p Allocator.
+  StringRef copyString(StringRef String, llvm::BumpPtrAllocator &Allocator);
+
----------------
Should this be a private member function? The overload that doesn't take in an allocator makes sense since it uses the `APISet`'s allocator, but this one does not seem like it should be publicly accessible, since it just ties the lifetime of the returned String to that of an arbitrary allocator?


================
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,
----------------
zixuw wrote:
> 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.
OK that is a nice change then, I wasn't aware that this is a thing. I was just complaining because it made the diff harder to parse for me ;)


================
Comment at: clang/lib/ExtractAPI/ExtractAPIConsumer.cpp:195
+    // the Symbol Graph format.
+    SymbolGraphSerializer SGSerializer(Visitor.getAPI());
+    SGSerializer.serialize(*OS);
----------------
zixuw wrote:
> 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.
Yeah that makes sense, we can do this as a follow up patch, definitely not urgent as SymbolGraph is the only use case at the moment. However, we should do this relatively soon so that the structure of the existing command line arguments is stable before we are fully done with this.


================
Comment at: clang/lib/ExtractAPI/Serialization/SymbolGraphSerializer.cpp:30
+/// at position \p Key.
 static void serializeObject(Object &Paren, StringRef Key,
                             Optional<Object> Obj) {
----------------
You don't need both static an anonymous namespace afaik.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122160