plotfi added a subscriber: kyulee1. plotfi added a comment. In D86049#3818923 <https://reviews.llvm.org/D86049#3818923>, @ahatanak wrote:
> In D86049#3818696 <https://reviews.llvm.org/D86049#3818696>, @plotfi wrote: > >> In D86049#3818435 <https://reviews.llvm.org/D86049#3818435>, @mwyman wrote: >> >>> In D86049#3816006 <https://reviews.llvm.org/D86049#3816006>, @plotfi wrote: >>> >>>> @dmaclach @ahatanak @mwyman How do things look from here? Do you want >>>> something for properties as well or would it be ok if we did this in a >>>> later commit? >>> >>> Huh, for some reason I thought when I'd last poked at using the >>> `visibility` attribute it wasn't allowed on ObjC methods, which is why I'd >>> suggested adding the enum on `objc_direct`, but as that no longer appears >>> to be the case yes I like this approach better. >> >> @dmaclach @mwyman I am also very happy with the fact that we can just reuse >> the regular `visibility` attribute. In the future we can decide on revised >> behavior for `hasMethodVisibilityDefault`. >> >> @ahatanak Do you have feedback on this? > > The visibility attribute changes look good to me. > > Do we have the answers to the last two questions John raised in > https://reviews.llvm.org/D86049#2255738? I think we should get it right the > first time since, once we make the direct methods visible, it'd be hard to > change where the null check or class initialization is done since that would > be an ABI change. Should we run experiments to measure the impact on code > size? I think it may be required to do thunk generation after all to do the null and init checks at the callee. What are your thoughts on this @ahatanak ? ================ Comment at: clang/lib/AST/Mangle.cpp:371 + OS << (MD->isInstanceMethod() ? '-' : '+'); + OS << (MD->hasMethodVisibilityDefault() ? '<' : '['); if (const auto *CID = MD->getCategory()) { ---------------- plotfi wrote: > ahatanak wrote: > > Sorry, I might have missed the discussion, but what's the reason we need > > this change in mangling? Is it because the linker cannot handle the > > standard mangling scheme? > Yeah. These are for ld and dyld. Not having a preceding underscore and the > square brackets causes problems. I need to ask @kyulee / @kyulee1 about the details. We went with these mangling changes some time ago and it is not fresh in my mind. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86049/new/ https://reviews.llvm.org/D86049 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
