[PATCH] D152770: [clang][ExtractAPI] Add support for Objective-C categories

[Zixu Wang via Phabricator via cfe-commits]

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