[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

lanza added inline comments. Comment at: clang/include/clang/AST/DeclObjC.h:2181 + /// This is true iff the protocol is tagged with the `objc_static_protocol` + /// attribute. kastiglione wrote: > This comment refers to the original spelling. Yes indeed,

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

kastiglione added inline comments. Comment at: clang/include/clang/AST/DeclObjC.h:2181 + /// This is true iff the protocol is tagged with the `objc_static_protocol` + /// attribute. This comment refers to the original spelling. Repository: rG LLVM Github

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG14f6bfcb52e7: [clang] Implement objc_non_runtime_protocol to

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

lanza updated this revision to Diff 295911. lanza added a comment. Clean clang-tidy warnings before landing Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75574/new/ https://reviews.llvm.org/D75574 Files: clang/include/clang/AST/DeclObjC.h

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGObjC.cpp:487 + + // Walk both lists to get the full set of implied protocols + llvm::DenseSet AllImpliedProtocols; lanza wrote: > rjmccall wrote: > > You should add something like ", including all

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

lanza updated this revision to Diff 295876. lanza added a comment. Comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75574/new/ https://reviews.llvm.org/D75574 Files: clang/include/clang/AST/DeclObjC.h clang/include/clang/Basic/Attr.td

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

lanza added inline comments. Comment at: clang/lib/CodeGen/CGObjC.cpp:487 + + // Walk both lists to get the full set of implied protocols + llvm::DenseSet AllImpliedProtocols; rjmccall wrote: > You should add something like ", including all the runtime

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

rjmccall added a comment. Thanks, that's a lot better. Just some minor suggestions. Comment at: clang/lib/CodeGen/CGObjC.cpp:477 + // If there are no non-runtime protocols then we can just stop now. + if (!NonRuntimePDs.size()) +return RuntimePds;

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

lanza requested review of this revision. lanza marked an inline comment as done. lanza added inline comments. Comment at: clang/lib/CodeGen/CGObjC.cpp:490 + llvm::UniqueVector FoundProtocols; + std::set DeclaredProtocols; + rjmccall wrote: > You should use

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

lanza updated this revision to Diff 295729. lanza added a comment. Update with John's suggestions Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75574/new/ https://reviews.llvm.org/D75574 Files: clang/include/clang/AST/DeclObjC.h

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGObjC.cpp:490 + llvm::UniqueVector FoundProtocols; + std::set DeclaredProtocols; + You should use llvm::DenseSet for this. But in general there are more sets here than I think you really need,

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

lanza added inline comments. Comment at: clang/lib/CodeGen/CGObjC.cpp:466 + for (; begin != end; ++begin) +AppendFirstRuntimeProtocols(*begin, PDs); + rjmccall wrote: > Should this make an effort to avoid declaring redundant bases? e.g. > > ``` >

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

lanza marked 5 inline comments as done. lanza added a comment. Fixed and added a test under the `REDUNDANCY` prefix. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75574/new/ https://reviews.llvm.org/D75574

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

lanza updated this revision to Diff 294207. lanza added a comment. Fix duplicate inheritance issue Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75574/new/ https://reviews.llvm.org/D75574 Files: clang/include/clang/AST/DeclObjC.h

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

rjmccall added a comment. Okay, the result of internal review is that we're comfortable with this feature. Reviewers brought up the point that it would be interesting to add some way to ensure unique emission of a protocol, so that protocols that can't be non-runtime could avoid bloating

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

theraven accepted this revision. theraven added a comment. This revision is now accepted and ready to land. Looks good to me, thank you! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75574/new/ https://reviews.llvm.org/D75574

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

lanza added a comment. @theraven @rjmccall should be ready for review whenever you guys are ready! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75574/new/ https://reviews.llvm.org/D75574 ___

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

lanza updated this revision to Diff 293944. lanza added a comment. Update Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75574/new/ https://reviews.llvm.org/D75574 Files: clang/include/clang/AST/DeclObjC.h clang/include/clang/Basic/Attr.td

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

lanza added a comment. > Hmm, I thought we actually just generated a bogus definition for the protocol > when it was forward-declared; really, this is better behavior that I > expected. Regardless, I don't think it's worthwhile to diagnose this more > strongly than a warning because of the

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

rjmccall added a comment. In D75574#2262622 , @lanza wrote: >> I don't think it'll actually error out at link time: protocol objects get >> emitted eagerly on use, cross-module linking is just a code-size >> optimization. This actually has caused

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

lanza added a comment. > I don't think it'll actually error out at link time: protocol objects get > emitted eagerly on use, cross-module linking is just a code-size > optimization. This actually has caused longstanding problems. But if it's just a forward declaration there's nothing to emit.

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

rjmccall added a comment. I don't think it'll actually error out at link time: protocol objects get emitted eagerly on use, cross-module linking is just a code-size optimization. This actually has caused longstanding problems. Anyway, I think it's fine to require forward declarations to have

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

lanza added a comment. A concern that has come up while rewriting this for the listed concerns is forward declared protocols that are defined as `non_runtime`. @protocol NonRuntimeProto; @interface Implementer : Root @end @implementation Implementer ... @end This compiles

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

rjmccall added a comment. In D75574#2202136 , @theraven wrote: > This feature looks generally useful. A few small suggestions: > > - This is really a way of transforming a formal protocol into an informal > protocol. Objective-C has had a convention of

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

theraven added a comment. This feature looks generally useful. A few small suggestions: - This is really a way of transforming a formal protocol into an informal protocol. Objective-C has had a convention of informal protocols since the '80s, but they're implemented as categories on the root

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

rjmccall added a reviewer: theraven. rjmccall added a comment. One thing that's come up so far: you generally need to be looking through non-runtime protocols, not ignoring them. This matters when non-runtime protocols inherit from ordinary protocols. It may be useful to provide a generic

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

lanza added a comment. No problem! Thank you, John! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75574/new/ https://reviews.llvm.org/D75574 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

rjmccall added a comment. Sorry, this slipped out of my mind. I've started the process internally. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75574/new/ https://reviews.llvm.org/D75574 ___

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

lanza added a comment. ping @rjmccall. Any update on a timeline for this review process? Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75574/new/ https://reviews.llvm.org/D75574 ___ cfe-commits

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

rjmccall added a comment. Sorry, I should've been more straight with you. I'm still happy to raise this internally, but this is a hectic time of the year at Apple, and I'm not actually going to raise it until after WWDC. It wouldn't be making it into the new Xcode release anyway.

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

lanza added a comment. @rjmccall Hey John, I sent the proposal to the addresses I was pointed to but haven't heard back in multiple weeks. Any update on this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75574/new/

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

lanza added a comment. > If someone writes up a short proposal for this, with motivation and impact, > we'd be happy to present it internally. I have a rough draft that I'll be touching up and submitting Monday most likely! Thanks, John! Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

rjmccall added a comment. If someone writes up a short proposal for this, with motivation and impact, we'd be happy to present it internally. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75574/new/ https://reviews.llvm.org/D75574

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

lanza updated this revision to Diff 252706. lanza added a comment. Rename and address some issues Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75574/new/ https://reviews.llvm.org/D75574 Files: clang/include/clang/AST/DeclObjC.h

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

lanza updated this revision to Diff 252707. lanza added a comment. Reword commit Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75574/new/ https://reviews.llvm.org/D75574 Files: clang/include/clang/AST/DeclObjC.h

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

manmanren added a comment. In D75574#1925808 , @rjmccall wrote: > This might also be interesting to @manmanren. Thank you, John! Manman Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75574/new/

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

rjmccall added a subscriber: manmanren. rjmccall added a comment. This might also be interesting to @manmanren. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75574/new/ https://reviews.llvm.org/D75574

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

lanza marked an inline comment as done. lanza added a comment. > Adding some more knowledgeable reviewers for comments on your RFC. I pointed > out a few minor nits, but I'll hold off on a technical review until the > ObjC-specific details are worked out and there is buy-in on the feature.

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

rjmccall added a comment. I'm fine with people developing a proposal for this openly, but it needs to be said that language changes cannot just be made in open-source; they have to go through the official language review process, which for Objective-C is an internal committee within Apple.

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

aaron.ballman added reviewers: aaron.ballman, rjmccall, erik.pilkington, MadCoder. aaron.ballman added a comment. Herald added a subscriber: dexonsmith. Adding some more knowledgeable reviewers for comments on your RFC. I pointed out a few minor nits, but I'll hold off on a technical review

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

lanza created this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Motivated by the new objc_direct attribute, this change aims to limit metadata generation from Protocols that the programmer knows isn't going to be used at runtime. This attribute simply causes