Came across code in Node.h <https://trac.webkit.org/browser/trunk/Source/WebCore/dom/Node.h?rev=249287#L619> that uses CompactPointerTuple to store a bitmask and does raw bitmask manipulation. We have a type safe convenience class called OptionSet<> that represents a bitmask that can abstract such operations away plus it is pretty printable. We should support CompactPointerTuple<..., OptionSet<...>>.
Created attachment 377647 [details] Patch and test
Comment on attachment 377647 [details] Patch and test View in context: https://bugs.webkit.org/attachment.cgi?id=377647&action=review > Source/WTF/wtf/CompactPointerTuple.h:71 > + static constexpr uint8_t toRaw(T value) { return value; } > + static constexpr T fromRaw(uint8_t value) { return static_cast<T>(value); } Just had a thought, maybe toByte() and fromByte() would be better names. Thoughts?
Created attachment 377648 [details] Patch and test
Another thought.... should update code to use std::byte.
(In reply to Daniel Bates from comment #4) > Another thought.... should update code to use std::byte. Not in this bug though assuming it is even feasible.
Comment on attachment 377648 [details] Patch and test View in context: https://bugs.webkit.org/attachment.cgi?id=377648&action=review > Tools/ChangeLog:11 > + * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj: Did you forget to svn add the new test?
(In reply to Sam Weinig from comment #6) > Comment on attachment 377648 [details] > Patch and test > > View in context: > https://bugs.webkit.org/attachment.cgi?id=377648&action=review > > > Tools/ChangeLog:11 > > + * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj: > > Did you forget to svn add the new test? Yep
Created attachment 377659 [details] Patch and test
Could make tests more comprehensive. Anyway this is a start as no tests for CompactPointerTuple were originally added
Created attachment 377734 [details] Patch and test
Created attachment 377928 [details] Patch
Created attachment 378391 [details] Patch
I didn't specialize std::is_integral because of: [[ None of the templates defined in <type_traits> may be specialized for a program-defined type, except for std::common_type. This includes the type traits and the class template std::integral_constant. ]] <https://unixpapa.com/js/testkey.html>
(In reply to Daniel Bates from comment #13) > I didn't specialize std::is_integral because of: > > [[ > None of the templates defined in <type_traits> may be specialized for a > program-defined type, except for std::common_type. This includes the type > traits and the class template std::integral_constant. > ]] > <https://unixpapa.com/js/testkey.html> * <https://en.cppreference.com/w/cpp/language/extending_std>
Created attachment 378739 [details] Patch
Created attachment 378741 [details] Patch
Comment on attachment 378741 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378741&action=review r=me with comment. > Source/WTF/wtf/CompactPointerTuple.h:77 > + template<typename T> struct ByteValueTypeAdapter { > + static constexpr uint8_t toByte(T value) { return value; } > + static constexpr T fromByte(uint8_t value) { return static_cast<T>(value); } > + }; > + > + template<typename U> struct ByteValueTypeAdapter<OptionSet<U>> { > + static constexpr uint8_t toByte(OptionSet<U> value) { return value.toRaw(); } > + static constexpr OptionSet<U> fromByte(uint8_t value) { return OptionSet<U>::fromRaw(value); } > + }; How about taking this as a template parameter of `CompactPointerTuple`? Like, template<typename PointerType, typename Type, typename Adaptor = ByteValueTypeAdapter<Type>> class CompactPointerTuple;
Comment on attachment 378741 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378741&action=review >> Source/WTF/wtf/CompactPointerTuple.h:77 >> + }; > > How about taking this as a template parameter of `CompactPointerTuple`? Like, > > template<typename PointerType, typename Type, typename Adaptor = ByteValueTypeAdapter<Type>> > class CompactPointerTuple; I can do this. Only reason not to is to avoid exposing ByteValueTypeAdapter in the WTF namespace....
Comment on attachment 378741 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378741&action=review >>> Source/WTF/wtf/CompactPointerTuple.h:77 >>> + }; >> >> How about taking this as a template parameter of `CompactPointerTuple`? Like, >> >> template<typename PointerType, typename Type, typename Adaptor = ByteValueTypeAdapter<Type>> >> class CompactPointerTuple; > > I can do this. Only reason not to is to avoid exposing ByteValueTypeAdapter in the WTF namespace.... Haha, I was trying to avoid exposing this struct, but accidentally put in public section. So, your suggestion is no different except for qualification.
Created attachment 380541 [details] Updated patch
Created attachment 380562 [details] To land
Committed r250930: <https://trac.webkit.org/changeset/250930>
<rdar://problem/56129500>
Reverted r250930 for reason: Broke watchOS Builds Committed r250943: <https://trac.webkit.org/changeset/250943>
Created attachment 380589 [details] Take 2: To Land
Committed r250945: <https://trac.webkit.org/changeset/250945>
It looks like this change, as well as the previous one that was rolled out, has broken 18 API tests on Debug for Mac and iOS. Build: https://build.webkit.org/builders/Apple%20Mojave%20Debug%20WK1%20%28Tests%29/builds/6157 They are crashing with this: ASSERTION FAILED: Enumerator is not a positive power of two. hasOneBitSet(static_cast<StorageType>(option)) /Volumes/Data/slave/mojave-debug/build
Reverted r250945 for reason: Broke 18 Debug API tests Committed r251008: <https://trac.webkit.org/changeset/251008>