Bug 214224 – Make hasher work with tuple-like classes

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

Make hasher work with tuple-like classes

[Sam Weinig]

Created attachment 404063 [details]
Patch

[Sam Weinig]

Created attachment 404065 [details]
Patch

[Darin Adler]

Comment on attachment 404065 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404065&action=review

Thanks for finishing the job. You can tell from all the FIXME that I tried this and couldn’t figure it out.

Where’s the part of this patch where you use this in Color?

> Source/WTF/ChangeLog:25
> +        adhear to WebKit style as IsTypeComplete<>.

adhere

> Source/WTF/wtf/StdLibExtras.h:424
> +// Based on 'Detecting in C++ whether a type is defined, part 3: SFINAE and incomplete types'
> +// <https://devblogs.microsoft.com/oldnewthing/20190710-00/?p=102678>
> +template<typename, typename = void> constexpr bool IsTypeComplete = false;
> +template<typename T> constexpr bool IsTypeComplete<T, std::void_t<decltype(sizeof(T))>> = true;

Why rename IsTypeComplete? If we’re just previewing C++20, could we use the real name and inject in the std namespace? Or maybe this is the way to go.

[Sam Weinig]

(In reply to Darin Adler from comment #3)
> Comment on attachment 404065 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404065&action=review
> 
> Thanks for finishing the job. You can tell from all the FIXME that I tried
> this and couldn’t figure it out.
> 
> Where’s the part of this patch where you use this in Color?

Since I have not landed that one yet (still needs a final r+), I figured I land this first, and land the fix for WTF first.

> 
> > Source/WTF/ChangeLog:25
> > +        adhear to WebKit style as IsTypeComplete<>.
> 
> adhere
> 
> > Source/WTF/wtf/StdLibExtras.h:424
> > +// Based on 'Detecting in C++ whether a type is defined, part 3: SFINAE and incomplete types'
> > +// <https://devblogs.microsoft.com/oldnewthing/20190710-00/?p=102678>
> > +template<typename, typename = void> constexpr bool IsTypeComplete = false;
> > +template<typename T> constexpr bool IsTypeComplete<T, std::void_t<decltype(sizeof(T))>> = true;
> 
> Why rename IsTypeComplete? If we’re just previewing C++20, could we use the
> real name and inject in the std namespace? Or maybe this is the way to go.

I don't think it's a c++20 thing, just a useful trait that Raymond Chen blogged about.

Thanks.

[Sam Weinig]

Committed r264270: <https://trac.webkit.org/changeset/264270>

[Radar WebKit Bug Importer]

<rdar://problem/65410178>

[Yusuke Suzuki]

Re-opened since this is blocked by bug 214228

[Alexey Proskuryakov]

Reverting due to broken build, failure details in Slack.

[Yusuke Suzuki]

Created attachment 404090 [details]
Patch + `constexpr` => `inline constexpr` template variable fix

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #9)
> Created attachment 404090 [details]
> Patch + `constexpr` => `inline constexpr` template variable fix

Internal one is built with this fix.

[Yusuke Suzuki]

Committed r264277: <https://trac.webkit.org/changeset/264277>

[Yusuke Suzuki]

Comment on attachment 404065 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404065&action=review

I've relanded the patch with `inline constexpr` fix, and note the rationale why this is necessary here :)

> Source/WTF/wtf/Hasher.h:110
> +template<typename, typename = void> constexpr bool HasBeginFunctionMember = false;
> +template<typename T> constexpr bool HasBeginFunctionMember<T, std::void_t<decltype(std::declval<T>().begin())>> = true;

We need `inline constexpr` to make this template-variable inline-variable (C++ is super complicated!)
https://stackoverflow.com/questions/14391272/does-constexpr-imply-inline

> Source/WTF/wtf/Hasher.h:129
> +template<typename T> constexpr bool HasGetFunctionMember<T, std::void_t<decltype(std::declval<T>().template get<0>())>> = true;

Ditto. Work-around with `inline constexpr`.

>> Source/WTF/wtf/StdLibExtras.h:424
>> +template<typename T> constexpr bool IsTypeComplete<T, std::void_t<decltype(sizeof(T))>> = true;
> 
> Why rename IsTypeComplete? If we’re just previewing C++20, could we use the real name and inject in the std namespace? Or maybe this is the way to go.

Ditto :)

[Darin Adler]

Comment on attachment 404065 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404065&action=review

>> Source/WTF/wtf/Hasher.h:110
>> +template<typename T> constexpr bool HasBeginFunctionMember<T, std::void_t<decltype(std::declval<T>().begin())>> = true;
> 
> We need `inline constexpr` to make this template-variable inline-variable (C++ is super complicated!)
> https://stackoverflow.com/questions/14391272/does-constexpr-imply-inline

This is the first time I have ever heard of an inline variable!

>>> Source/WTF/wtf/StdLibExtras.h:424
>>> +template<typename T> constexpr bool IsTypeComplete<T, std::void_t<decltype(sizeof(T))>> = true;
>> 
>> Why rename IsTypeComplete? If we’re just previewing C++20, could we use the real name and inject in the std namespace? Or maybe this is the way to go.
> 
> Ditto :)

Sam clarified that is_type_complete is not in C++20.

[Yusuke Suzuki]

Comment on attachment 404065 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404065&action=review

>>> Source/WTF/wtf/Hasher.h:110
>>> +template<typename T> constexpr bool HasBeginFunctionMember<T, std::void_t<decltype(std::declval<T>().begin())>> = true;
>> 
>> We need `inline constexpr` to make this template-variable inline-variable (C++ is super complicated!)
>> https://stackoverflow.com/questions/14391272/does-constexpr-imply-inline
> 
> This is the first time I have ever heard of an inline variable!

inline variable is one of the best feature in C++17! Sometimes we, C++ developers, saw super weird linker errors about `static XXX::value` variables defined in the header because,

1. One translation unit is accessing it via reference.
2. One translation unit is building it as a constant value and do not generate a data for that.

Then linker causes link failures because (1) cannot find the referenced data. Inline variable solves this problem by ensuring that this variable is not referenced like (1).