Bug 165093 – Introduce StringImpl::StaticStringImpl with constexpr constructor

• Component: Web Template Framework
• Status: REOPENED

[Yusuke Suzuki]

Add constexpr StringImpl constructor. It allows us to initialize static StringImpl without global constructor.
And let's use it to inline StringImpl::empty()!

[Yusuke Suzuki]

Created attachment 295499 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 295588 [details]
Patch

[Yusuke Suzuki]

Created attachment 295589 [details]
Patch

[Yusuke Suzuki]

Created attachment 295690 [details]
Patch

[Darin Adler]

Comment on attachment 295690 [details]
Patch

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

> Source/WTF/wtf/Hasher.h:299
> +// FIXME: WebKitGTK+ requires GCC 4.9, which does not support relaxed constexpr in C++14.
> +// Once it is supported, we can simplify the implementation.

I would have written a different comment here.

- I don’t think we need to specify WebKitGTK+ here, just mention that the project still supports GCC 4.9.
- I would have been more specific about what kind of simplification we are talking about.

// FIXME: This code limits itself to the older, more limited C++11 constexpr capabilities, using
// recursion instead of looping, for example. Would be nice to rewrite this in a simpler way
// once we no longer need to support compilers like GCC 4.9 that do not yet support it.

> Source/WTF/wtf/Hasher.h:300
> +class ConstexprStringHasher {

Why does this need to be a separate class? Can we just make constexpr functions named computeLiteralHash and computeLiteralHashAndMaskTop8Bits in the StringHasher class? Whatever names we need for the private functions.

> Source/WTF/wtf/Hasher.h:343
> +    // This avoids ever returning a hash code of 0, since that is used to
> +    // signal "hash not computed yet". Setting the high bit maintains
> +    // reasonable fidelity to a hash code of 0 because it is likely to yield
> +    // exactly 0 when hash lookup masks out the high bits.

It’s annoying to have this whole comment repeated twice. Can we find a way to just have this refer to the code above, or perhaps even find a way to share the code? Code should be able to call a constexpr function with a non-constexpr argument.

> Source/WTF/wtf/Hasher.h:357
> +        // Reserving space from the high bits for flags preserves most of the hash's
> +        // value, since hash lookup typically masks out the high bits anyway.

Same thought. It’s strange to have this comment here when the real issue is that this needs to exactly match the behavior of the code above.

> Source/WTF/wtf/Hasher.h:420
>  using WTF::IntegerHasher;
>  using WTF::StringHasher;
> +using WTF::ConstexprStringHasher;

Please sort alphabetically.

> Source/WTF/wtf/text/StringImpl.h:-869
> -    WTF_EXPORT_PRIVATE static StringImpl* null();

I don’t think this function should be named null, and it’s not great to make the function public while keeping the name null. For StringImpl*, the null value is nullptr. I believe you added StringImpl::null, adding yet another different kind of null string, to help implement Symbol. But it’s very confusing to have a function named null() that returns it; the name should be more specific to the specific use. I think it’s a special kind of empty string. I don’t think we should call it null.

Let me give you an example of why it’s confusing: if we called String(StringImpl::null()).isNull() we would get false.

> Source/WTF/wtf/text/StringImpl.h:931
> +static_assert(sizeof(StringImpl) == sizeof(StringImpl::StaticStringImpl), "sizeof(StringImpl) should be the same to sizeof(StaticStringImpl)");

The comment here is simply repeats exactly what the expression says. I would use "" or write a comment that explains wy.

> Tools/ChangeLog:3
> +        [WTF] Introduce StringImpl::StaticStringImpl with constexpr constructor

No good reason for the [WTF] prefix on the bug name.

[Yusuke Suzuki]

Comment on attachment 295690 [details]
Patch

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

Thanks

>> Source/WTF/wtf/Hasher.h:299
>> +// Once it is supported, we can simplify the implementation.
> 
> I would have written a different comment here.
> 
> - I don’t think we need to specify WebKitGTK+ here, just mention that the project still supports GCC 4.9.
> - I would have been more specific about what kind of simplification we are talking about.
> 
> // FIXME: This code limits itself to the older, more limited C++11 constexpr capabilities, using
> // recursion instead of looping, for example. Would be nice to rewrite this in a simpler way
> // once we no longer need to support compilers like GCC 4.9 that do not yet support it.

Sounds nice. Fixed.

>> Source/WTF/wtf/Hasher.h:300
>> +class ConstexprStringHasher {
> 
> Why does this need to be a separate class? Can we just make constexpr functions named computeLiteralHash and computeLiteralHashAndMaskTop8Bits in the StringHasher class? Whatever names we need for the private functions.

OK, imported it. Once C++14 constexpr is enabled, we can unify these things.

>> Source/WTF/wtf/Hasher.h:343
>> +    // exactly 0 when hash lookup masks out the high bits.
> 
> It’s annoying to have this whole comment repeated twice. Can we find a way to just have this refer to the code above, or perhaps even find a way to share the code? Code should be able to call a constexpr function with a non-constexpr argument.

Share avalancheBits, finalize, and avoidZero phases with StringHasher.

>> Source/WTF/wtf/Hasher.h:357
>> +        // value, since hash lookup typically masks out the high bits anyway.
> 
> Same thought. It’s strange to have this comment here when the real issue is that this needs to exactly match the behavior of the code above.

Ditto.

>> Source/WTF/wtf/Hasher.h:420
>> +using WTF::ConstexprStringHasher;
> 
> Please sort alphabetically.

Fixed.

>> Source/WTF/wtf/text/StringImpl.h:-869
>> -    WTF_EXPORT_PRIVATE static StringImpl* null();
> 
> I don’t think this function should be named null, and it’s not great to make the function public while keeping the name null. For StringImpl*, the null value is nullptr. I believe you added StringImpl::null, adding yet another different kind of null string, to help implement Symbol. But it’s very confusing to have a function named null() that returns it; the name should be more specific to the specific use. I think it’s a special kind of empty string. I don’t think we should call it null.
> 
> Let me give you an example of why it’s confusing: if we called String(StringImpl::null()).isNull() we would get false.

It seems that this is introduced in https://trac.webkit.org/changeset/198168. The reason why we introduced it seems that we need to distinguish Symbol() from Symbol("").
It seems a bit dangerous to me. It breaks some invariant that empty string is always StringImpl::empty().

Rather than that, I think SymbolImpl should have a flag for that. I'll refactor SymbolImpl and drop this StringImpl::null() later. In the meantime, I'll leave it as is.
https://bugs.webkit.org/show_bug.cgi?id=165247

>> Source/WTF/wtf/text/StringImpl.h:931
>> +static_assert(sizeof(StringImpl) == sizeof(StringImpl::StaticStringImpl), "sizeof(StringImpl) should be the same to sizeof(StaticStringImpl)");
> 
> The comment here is simply repeats exactly what the expression says. I would use "" or write a comment that explains wy.

Fixed.

>> Tools/ChangeLog:3
>> +        [WTF] Introduce StringImpl::StaticStringImpl with constexpr constructor
> 
> No good reason for the [WTF] prefix on the bug name.

Fixed.

[Yusuke Suzuki]

Committed r209179: <http://trac.webkit.org/changeset/209179>

[Yusuke Suzuki]

Reopening to attach new patch.

[Yusuke Suzuki]

Created attachment 297942 [details]
C++14 relaxed constexpr

WIP