This is an archive of the discontinued LLVM Phabricator instance.

[clang][extract-api] Add Objective-C interface support
ClosedPublic

Authored by zixuw on Mar 24 2022, 6:22 PM.

Details

Summary

Add support for Objective-C interface declarations in ExtractAPI.

Diff Detail

Event Timeline

zixuw created this revision.Mar 24 2022, 6:22 PM
Herald added a project: Restricted Project. · View Herald Transcript
zixuw requested review of this revision.Mar 24 2022, 6:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2022, 6:22 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
zixuw updated this revision to Diff 418286.Mar 25 2022, 11:53 AM

Move the Language fix out into a separate patch: D122495

zixuw updated this revision to Diff 418342.Mar 25 2022, 3:17 PM

No code change, amend commit message.

zixuw edited the summary of this revision. (Show Details)Mar 25 2022, 3:19 PM
dang added inline comments.Mar 28 2022, 1:37 PM
clang/lib/ExtractAPI/DeclarationFragments.cpp
472–650

If I am not mistaken, these do the same thing but need separate overloads because the types don't share a common super class. If this is true we should still remove the duplication here. There are two ways of doing this AFAIK:

  1. Have a template helper function that does the work and call it from both overloads
  2. Create a "union" type that contains either of the two alternatives and delegates the method calls appropriately.
clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
245

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.

455

I think we need to allocate these string in the StringAllocator.

dang added inline comments.Mar 28 2022, 2:02 PM
clang/lib/ExtractAPI/Serialization/SymbolGraphSerializer.cpp
376

this should probably be more explicit to keep in line with how things are currently done. Maybe something like "instance.variable"

zixuw updated this revision to Diff 418738.EditedMar 28 2022, 5:13 PM
zixuw marked 4 inline comments as done.
  • Address review comments:
    • Use template to reuse logic for building function signatures for FunctionDecl and ObjCMethodDecl.
  • Move the change of objc_interface.m test in this patch from D122511.
clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
245

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.

455

Same as above.

clang/lib/ExtractAPI/Serialization/SymbolGraphSerializer.cpp
376

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?

dang added inline comments.Mar 29 2022, 3:34 AM
clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
245

Yeah sounds good!

clang/lib/ExtractAPI/Serialization/SymbolGraphSerializer.cpp
376

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.

dang added inline comments.Mar 29 2022, 5:31 AM
clang/lib/ExtractAPI/DeclarationFragments.cpp
642–648

Do we need to explicitly instantiate them? Wouldn't they get instantiated as needed?

clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
245

I have looked into it a bit more and the ASTContext will still be live at that point (it gets deallocated right after we will be doing the serialization) so this is fine.

dang added inline comments.Mar 29 2022, 5:53 AM
clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
245

I thought it would still make sense to split the macro patch in two so here we go: https://reviews.llvm.org/D122648

dang requested changes to this revision.Mar 29 2022, 6:16 AM
dang added inline comments.
clang/lib/ExtractAPI/DeclarationFragments.cpp
660

We shouldn't be appending the +/- here. We can discuss this more offline

This revision now requires changes to proceed.Mar 29 2022, 6:16 AM
zixuw marked an inline comment as done.Mar 29 2022, 10:31 AM
zixuw added inline comments.
clang/lib/ExtractAPI/DeclarationFragments.cpp
642–648

Yeah since we are implementing the templated method here instead of in the header file, we need to explicitly instantiate it so that the symbol ends up in DeclarationFragments.o and picked up by the linker.

clang/lib/ExtractAPI/Serialization/SymbolGraphSerializer.cpp
376

For my understanding, struct fields got named "property" because they behave similar as properties, and there are no other possible member kinds in a struct to overload that concept. While in an Objective-C container, there are actual literal Objective-C "properties." How do we distinguish between ivars and properties if they're the same kind?

We might also reconsider the kind for struct fields as "instance property" isn't really accurate and largely Objective-C-ish.

dang accepted this revision.Mar 29 2022, 12:12 PM
dang added inline comments.
clang/lib/ExtractAPI/DeclarationFragments.cpp
642–648

Oh yeah right of course.

660

Nevermind We do need them here.

This revision is now accepted and ready to land.Mar 29 2022, 12:12 PM
This revision was landed with ongoing or failed builds.Mar 29 2022, 2:29 PM
This revision was automatically updated to reflect the committed changes.
zixuw marked an inline comment as done.