Switch WTF::bridge_cast to use type traits. This replaces some macro-generated code with template code. Not sure the new code is more readable, but error messages should be much less verbose. Defining the type traits in WebKit will also let us remove some duplicate code in Safari. This is based on some code written in November 2011 by Anders Carlsson.
<rdar://problem/84050614>
Created attachment 440693 [details] Patch v1
Darin, I tried my best to use this to fix the return type of RetainPtr<>::bridgingAutorelease(), but I couldn't figure out how to make this work: #ifdef __OBJC__ + using BridgingPtrType = typename std::conditional_t<!std::is_convertible_v<T, id> && IsTypeComplete<CFTollFreeBridgingTraits<T>>, typename CFTollFreeBridgingTraits<T>::BridgedType, id>; using HelperPtrType = typename std::conditional_t<std::is_convertible_v<T, id> && !std::is_same_v<T, id>, std::remove_pointer_t<T>, T>; #else + using BridgingPtrType = id; using HelperPtrType = PtrType; #endif The challenge is that even if `IsTypeComplete<CFTollFreeBridgingTraits<T>` is false, `typename CFTollFreeBridgingTraits<T>::BridgedType` must still evaluate to something (even if it's not used). Or at least that's the conclusion I came to when I tried it.
I haven’t lost confidence that we can do it yet. We do not want bridgingAutorelease to do less type checking when the type is not complete, so I think that part of the solution isn’t heading in the right direction. What we want instead is that calls to bridgingAutorelease fail to compile successfully, maybe because of SFINAE, when the type is not complete. So this means we may not be able to use a using at the class level; the type expression may need to be entirely within the bridgingAutorelease function definition.
(In reply to Darin Adler from comment #5) > I haven’t lost confidence that we can do it yet. > > We do not want bridgingAutorelease to do less type checking when the type is > not complete, so I think that part of the solution isn’t heading in the > right direction. Okay. > What we want instead is that calls to bridgingAutorelease fail to compile > successfully, maybe because of SFINAE, when the type is not complete. So > this means we may not be able to use a using at the class level; the type > expression may need to be entirely within the bridgingAutorelease function > definition. I don't think SFINAE will work because we can't make an instance method of RetainPtr<> optional (that I'm aware of), but maybe there's an SFINAE pattern I'm not thinking of. We'd have to move bridgingAutorelease() out of the RetainPtr<> class (to a stand-alone function) to use SFINAE. This does work: template<typename T> inline id RetainPtr<T>::bridgingAutorelease() { static_assert(!std::is_convertible_v<PtrType, id> && IsTypeComplete<CFTollFreeBridgingTraits<PtrType>>, "bridgingAutorelease() can only be used for toll-free bridged CF types."); return CFBridgingRelease(leakRef()); } And it already caught an "error": WebKitBuild/Debug/usr/local/include/wtf/RetainPtr.h:223:5: error: static_assert failed due to requirement 'IsTypeComplete<WTF::CFTollFreeBridgingTraits<const __AXTextMarker *>>' "bridgingAutorelease() can only be used for toll-free bridged CF types." static_assert(!std::is_convertible_v<PtrType, id> && IsTypeComplete<CFTollFreeBridgingTraits<PtrType>>, "bridgingAutorelease() can only be used for toll-free bridged CF types."); ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from WebKitBuild/Debug/DerivedSources/WebCore/unified-sources/UnifiedSource5-mm.mm:5: ./accessibility/mac/AXObjectCacheMac.mm:494:38: note: in instantiation of member function 'WTF::RetainPtr<const __AXTextMarker *>::bridgingAutorelease' requested here [change setObject:textMarker.bridgingAutorelease() forKey:NSAccessibilityTextChangeValueStartMarker]; ^ 1 error generated. Is this the type of issue we want to catch? I thought it was generally okay to autorelease CF objects. Looks like this should probably just be a .get() with a bridge_id_cast() to prevent the autorelease in the first place: static void addTextMarkerFor(NSMutableDictionary* change, AXCoreObject& object, HTMLTextFormControlElement& textControl) { if (auto textMarker = [object.wrapper() textMarkerForFirstPositionInTextControl:textControl]) - [change setObject:textMarker.bridgingAutorelease() forKey:NSAccessibilityTextChangeValueStartMarker]; + [change setObject:bridge_id_cast(textMarker.get()) forKey:NSAccessibilityTextChangeValueStartMarker]; }
Created attachment 440712 [details] Patch v2
Review ping.
(In reply to David Kilzer (:ddkilzer) from comment #6) > Is this the type of issue we want to catch? I thought it was generally okay > to autorelease CF objects. Once bridgingAutorelease chooses the type correctly, it might need to be replaced by two different functions, for the same reason we added both bridge_cast and bridge_id_cast; the former can be used when the class is toll-free bridged with a specific Objective-C class, while the latter can be used for any CF object that we choose to use with Objective-C, even if there is no Objective-C counterpart. Thinking about this, I want us to think about which cast function one should use in the other direction, to convert the "id" back to a CoreFoundation type. For object ownership we would use adoptNS, but then how would we convert the RetainPtr<id> into RetainPtr<AXTextMarkerRef>. It seems this particular case calls for the bridge_id_cast style. > Looks like this should probably just be a .get() with a bridge_id_cast() to > prevent the autorelease in the first place Yes, this code does not need autorelease; it needs bridging. > - [change setObject:textMarker.bridgingAutorelease() > forKey:NSAccessibilityTextChangeValueStartMarker]; > + [change setObject:bridge_id_cast(textMarker.get()) > forKey:NSAccessibilityTextChangeValueStartMarker]; I’m noticing that this will be slightly suboptimal under ARC, requiring an extra retain/release pair. Kind of annoying that RetainPtr messes up ARC optimizations.
Comment on attachment 440712 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=440712&action=review > Source/WTF/wtf/RetainPtr.h:224 > + static_assert(!std::is_convertible_v<PtrType, id> && IsTypeComplete<CFTollFreeBridgingTraits<PtrType>>, "bridgingAutorelease() may only be used for toll-free bridged CF types."); The idea behind my saying "base the return type on the type that is bridged" was not to generate compile time errors when we wanted to bridge to just "id". So I’m not sure it’s worth making this change yet. > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4134 > - return [protectedSelf nextTextMarkerForCharacterOffset:characterOffset].bridgingAutorelease(); > + return bridge_id_cast([protectedSelf nextTextMarkerForCharacterOffset:characterOffset]); Is this really right? We need autorelease here. Does bridge_id_cast implicitly do an autorelease? > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4149 > - return [protectedSelf previousTextMarkerForCharacterOffset:characterOffset].bridgingAutorelease(); > + return bridge_id_cast([protectedSelf previousTextMarkerForCharacterOffset:characterOffset]); Same.
Comment on attachment 440712 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=440712&action=review >> Source/WTF/wtf/RetainPtr.h:224 >> + static_assert(!std::is_convertible_v<PtrType, id> && IsTypeComplete<CFTollFreeBridgingTraits<PtrType>>, "bridgingAutorelease() may only be used for toll-free bridged CF types."); > > The idea behind my saying "base the return type on the type that is bridged" was not to generate compile time errors when we wanted to bridge to just "id". So I’m not sure it’s worth making this change yet. Okay, I'll revert this. We don't have to solve fixing the return type of bridgingAutorelease() before landing the patch, correct? >> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4134 >> + return bridge_id_cast([protectedSelf nextTextMarkerForCharacterOffset:characterOffset]); > > Is this really right? We need autorelease here. Does bridge_id_cast implicitly do an autorelease? I'm pretty sure this lambda returns a RetainPtr<id>, so this is way more efficient than autoreleasing it and then re-retaining it, correct? return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([&textMarker, protectedSelf = retainPtr(self)] () -> RetainPtr<id> { Maybe I amI missing something? Is there some unexpected requirement that this be autoreleased before returning it? >> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4149 >> + return bridge_id_cast([protectedSelf previousTextMarkerForCharacterOffset:characterOffset]); > > Same. Same: return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([&textMarker, protectedSelf = retainPtr(self)] () -> RetainPtr<id> {
Comment on attachment 440712 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=440712&action=review >>> Source/WTF/wtf/RetainPtr.h:224 >>> + static_assert(!std::is_convertible_v<PtrType, id> && IsTypeComplete<CFTollFreeBridgingTraits<PtrType>>, "bridgingAutorelease() may only be used for toll-free bridged CF types."); >> >> The idea behind my saying "base the return type on the type that is bridged" was not to generate compile time errors when we wanted to bridge to just "id". So I’m not sure it’s worth making this change yet. > > Okay, I'll revert this. We don't have to solve fixing the return type of bridgingAutorelease() before landing the patch, correct? That’s right. A separate thought that we can pursue in its own time. >>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4134 >>> + return bridge_id_cast([protectedSelf nextTextMarkerForCharacterOffset:characterOffset]); >> >> Is this really right? We need autorelease here. Does bridge_id_cast implicitly do an autorelease? > > I'm pretty sure this lambda returns a RetainPtr<id>, so this is way more efficient than autoreleasing it and then re-retaining it, correct? > > return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([&textMarker, protectedSelf = retainPtr(self)] () -> RetainPtr<id> { > > Maybe I amI missing something? Is there some unexpected requirement that this be autoreleased before returning it? I did not see retrieveAutoreleasedValueFromMainThread, clearly that’s what does the autorelease. Thanks, my mistake.
Created attachment 440870 [details] Patch for landing
Comment on attachment 440870 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=440870&action=review > Source/WTF/wtf/cocoa/TollFreeBridging.h:42 > +template<> struct CFTollFreeBridgingTraits<CFClassName##Ref> { typedef NSClassName *BridgedType; }; \ > +template<> struct NSTollFreeBridgingTraits<NSClassName> { typedef CFClassName##Ref BridgedType; }; Realized that to be more modern we could use "using" instead of typedef here. > Source/WTF/wtf/cocoa/TollFreeBridging.h:60 > +template<> struct CFTollFreeBridgingTraits<CFBooleanRef> { typedef NSNumber *BridgedType; }; Ditto.
Created attachment 440970 [details] Patch for landing v2
Comment on attachment 440970 [details] Patch for landing v2 Marking cq+ since the only changes from "Patch for landing" were to change `typedef` to `using`, which would have appeared as a build failure if they were an issue.
Committed r284038 (242868@main): <https://commits.webkit.org/242868@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 440970 [details].