zixuw added inline comments.

================
Comment at: clang/lib/ExtractAPI/ExtractAPIConsumer.cpp:245
+    if (const auto *SuperClassDecl = Decl->getSuperClass()) {
+      SuperClass.Name = SuperClassDecl->getObjCRuntimeNameAsString();
+      SuperClass.USR = API.recordUSR(SuperClassDecl);
----------------
dang wrote:
> Shouldn't we be recording this string in the StringAllocator for reuse later. 
> I have a patch in progress for macro support that will do the serialization 
> once the AST isn't available anymore so we really shouldn't be taking in 
> StringRefs for stuff in the AST.
Right but that only makes sense once we kill the AST before serialization 
right? I mean I'm happy to wrap these in `copyString` now if we know for sure 
that we're doing that in a later patch. But it feels beyond the scope of this 
particular patch. Especially that we'd also need to go through the previous 
code to copy the names of global records, enums, etc. anyway.

I feel like that the change to surface `APISet` and punt serialization should 
be separated out from the macro change. And we can wrap all the name strings in 
that patch so that it's a meaningful atomic commit.


================
Comment at: clang/lib/ExtractAPI/ExtractAPIConsumer.cpp:455
+    for (const auto *Protocol : Protocols)
+      Container->Protocols.emplace_back(Protocol->getName(),
+                                        API.recordUSR(Protocol));
----------------
dang wrote:
> I think we need to allocate these string in the StringAllocator.
Same as above.


================
Comment at: clang/lib/ExtractAPI/Serialization/SymbolGraphSerializer.cpp:375
+  case APIRecord::RK_ObjCIvar:
+    Kind["identifier"] = AddLangPrefix("ivar");
+    Kind["displayName"] = "Instance Variable";
----------------
dang wrote:
> this should probably be more explicit to keep in line with how things are 
> currently done. Maybe something like "instance.variable"
Right, naming completely new things here so worth the discussion.
To keep in line with the current convention, I don't see instance methods 
having an `instance.` prefix. It's just `method` as opposed to `type.method`.
Also global variable is just `var`, if we really need to add the `instance.` 
prefix (which I still don't think makes much sense for the above reason), 
wouldn't it be `instance.var`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122446

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to