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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
