dang 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);
----------------
zixuw wrote:
> 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.
Yeah sounds good!
================
Comment at: clang/lib/ExtractAPI/Serialization/SymbolGraphSerializer.cpp:375
+ case APIRecord::RK_ObjCIvar:
+ Kind["identifier"] = AddLangPrefix("ivar");
+ Kind["displayName"] = "Instance Variable";
----------------
zixuw wrote:
> 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`?
Actually, now that I think about it, we should just make them properties. There
is already a precedent of doing that for struct fields, and ivars are pretty
much the equivalent of a struct field for an interface.
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
