Bug 201316 – Add support for CompactPointerTuple<..., OptionSet<...>>

• Status: REOPENED
• Component: Web Template Framework

[Daniel Bates]

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<...>>.

[Daniel Bates]

Created attachment 377647 [details]
Patch and test

[Daniel Bates]

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?

[Daniel Bates]

Created attachment 377648 [details]
Patch and test

[Daniel Bates]

Another thought.... should update code to use std::byte.

[Daniel Bates]

(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.

[Sam Weinig]

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?

[Daniel Bates]

(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

[Daniel Bates]

Created attachment 377659 [details]
Patch and test

[Daniel Bates]

Could make tests more comprehensive. Anyway this is a start as no tests for CompactPointerTuple were originally added

[Daniel Bates]

Created attachment 377734 [details]
Patch and test

[Daniel Bates]

Created attachment 377928 [details]
Patch

[Daniel Bates]

Created attachment 378391 [details]
Patch

[Daniel Bates]

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>

[Daniel Bates]

(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>

[Daniel Bates]

Created attachment 378739 [details]
Patch

[Daniel Bates]

Created attachment 378741 [details]
Patch

[Yusuke Suzuki]

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;

[Daniel Bates]

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....

[Daniel Bates]

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.

[Daniel Bates]

Created attachment 380541 [details]
Updated patch

[Daniel Bates]

Created attachment 380562 [details]
To land

[Daniel Bates]

Committed r250930: <https://trac.webkit.org/changeset/250930>

[Radar WebKit Bug Importer]

<rdar://problem/56129500>

[Russell Epstein]

Reverted r250930 for reason:

Broke watchOS Builds

Committed r250943: <https://trac.webkit.org/changeset/250943>

[Daniel Bates]

Created attachment 380589 [details]
Take 2: To Land

[Daniel Bates]

Committed r250945: <https://trac.webkit.org/changeset/250945>

[Truitt Savell]

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

[Jonathan Bedard]

Reverted r250945 for reason:

Broke 18 Debug API tests

Committed r251008: <https://trac.webkit.org/changeset/251008>