zixuw added a comment.
Thanks for the patch!
Like I mentioned in the inline comments, I'm not sure I understand the need of
the "abstract" (in the sense that it's not really a concrete API) subtype of
`APIRecord`. The design doesn't feel right within the existing structure.
If I recall the original issue correctly the problem is that a category might
extend an interface that is not declared within the current compilation unit
and the SymbolGraph output has references to an USR it cannot resolve. But
that's why we have the `SymbolReference Interface` for the category record, and
all the information we could possibly have from extract-api for that interface
is already in the `SymbolReference`. Couldn't we use that to design an output
format to indicate the external reference?
Also a side note that the word "module" has been overloaded, especially within
LLVM/clang where "module" is used for clang modules or C++ modules. I would
avoid creating a `*ModuleRecord` without qualifying it like `*ExtractAPIModule`
and providing documentation to clarify the meaning.
================
Comment at: clang/include/clang/ExtractAPI/API.h:148-150
+ APIRecord(RecordKind Kind, StringRef USR, StringRef Name)
+ : USR(USR), Name(Name), Kind(Kind) {}
+
----------------
I would really try to avoid patching the base `APIRecord` to get ways of
creating "fake" records. The API records in the API set should really be all
concrete symbols mapping back to an entity that is an API.
This doesn't feel right and might open up to future bad designs and bugs.
================
Comment at: clang/include/clang/ExtractAPI/API.h:497-510
+struct ObjCCategoryModuleRecord : APIRecord {
+ // ObjCCategoryRecord%s are stored in and owned by APISet.
+ SmallVector<ObjCCategoryRecord *> Categories;
+
+ ObjCCategoryModuleRecord(StringRef USR, StringRef Name)
+ : APIRecord(RK_ObjCCategoryModule, USR, Name) {}
+
----------------
I'm still not convinced by this concept and design. Why do we need to have a
subtype of `APIRecord` to represent relationships with external types from
another "module"? We have `SymbolReference` for that purpose with the name and
USR information.
Even if this is necessary, the name is extremely confusing. By the look of the
rest of the change and the test cases, I believe this is meant to represent the
external *interface* that a category is extending, not a category.
================
Comment at: clang/include/clang/ExtractAPI/ExtractAPIVisitor.h:128-135
+template <typename T>
+static bool isExternalCategory(const T &Records, const StringRef &Name) {
+ for (const auto &Record : Records) {
+ if (Name == Record.second.get()->Name)
+ return false;
+ }
+ return true;
----------------
This looks very concerning..
It's way too generic for its name and it's not doing what it's describing.
By the look of the call site below, this seems to be supposed to check if an
*interface*, not category, is recorded in this API set. There's no reason to
have a separate template function for that. You can just do the check inline
and also probably covered by one of the existing llvm STLExtra utilities.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D152770/new/
https://reviews.llvm.org/D152770
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
