WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
231467
Switch WTF::bridge_cast to use type traits
https://bugs.webkit.org/show_bug.cgi?id=231467
Summary
Switch WTF::bridge_cast to use type traits
David Kilzer (:ddkilzer)
Reported
2021-10-08 17:37:40 PDT
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.
Attachments
Patch v1
(8.53 KB, patch)
2021-10-08 17:40 PDT
,
David Kilzer (:ddkilzer)
no flags
Details
Formatted Diff
Diff
Patch v2
(13.44 KB, patch)
2021-10-09 10:19 PDT
,
David Kilzer (:ddkilzer)
darin
: review+
Details
Formatted Diff
Diff
Patch for landing
(12.25 KB, patch)
2021-10-11 19:14 PDT
,
David Kilzer (:ddkilzer)
no flags
Details
Formatted Diff
Diff
Patch for landing v2
(12.25 KB, patch)
2021-10-12 12:36 PDT
,
David Kilzer (:ddkilzer)
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2021-10-08 17:38:20 PDT
<
rdar://problem/84050614
>
David Kilzer (:ddkilzer)
Comment 2
2021-10-08 17:40:47 PDT
Created
attachment 440693
[details]
Patch v1
David Kilzer (:ddkilzer)
Comment 3
2021-10-08 17:48:36 PDT
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.
Darin Adler
Comment 4
2021-10-08 18:09:41 PDT
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.
Darin Adler
Comment 5
2021-10-08 18:09:41 PDT
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.
David Kilzer (:ddkilzer)
Comment 6
2021-10-08 20:53:08 PDT
(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]; }
David Kilzer (:ddkilzer)
Comment 7
2021-10-09 10:19:50 PDT
Created
attachment 440712
[details]
Patch v2
David Kilzer (:ddkilzer)
Comment 8
2021-10-11 08:50:11 PDT
Review ping.
Darin Adler
Comment 9
2021-10-11 15:00:24 PDT
(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.
Darin Adler
Comment 10
2021-10-11 16:50:50 PDT
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.
David Kilzer (:ddkilzer)
Comment 11
2021-10-11 17:46:51 PDT
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> {
Darin Adler
Comment 12
2021-10-11 17:48:45 PDT
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.
David Kilzer (:ddkilzer)
Comment 13
2021-10-11 19:14:19 PDT
Created
attachment 440870
[details]
Patch for landing
Darin Adler
Comment 14
2021-10-12 08:36:29 PDT
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.
David Kilzer (:ddkilzer)
Comment 15
2021-10-12 12:36:27 PDT
Created
attachment 440970
[details]
Patch for landing v2
David Kilzer (:ddkilzer)
Comment 16
2021-10-12 13:52:30 PDT
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.
EWS
Comment 17
2021-10-12 14:56:59 PDT
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]
.
David Kilzer (:ddkilzer)
Comment 18
2022-01-16 13:46:25 PST
(In reply to EWS from
comment #17
)
> Committed
r284038
(
242868@main
): <
https://commits.webkit.org/242868@main
>
Follow-up fix to add TollFreeBridging.h to the WTF Xcode project: Committed
r288075
: <
https://commits.webkit.org/r288075
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug