Bug 217838 – Extract StringImpl::computeHashAndFlags() function for passing to StringImplShape constructor

• Status: NEW
• Component: Web Template Framework

[David Kilzer (:ddkilzer)]

Extract StringImpl::computeHashAndFlags() function for passing to StringImplShape constructor.

This is a follow-on to Darin's comments in Bug 217805 Comment #9.

[David Kilzer (:ddkilzer)]

Created attachment 411604 [details]
Patch v1

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #1)
> Created attachment 411604 [details]
> Patch v1

LOL...posted this patch too soon.  Fix for Bug 217805 hasn't landed yet.

[David Kilzer (:ddkilzer)]

Created attachment 411606 [details]
Patch v1 redo

[Darin Adler]

Comment on attachment 411606 [details]
Patch v1 redo

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

> Source/WTF/wtf/text/StringImpl.h:213
> +    static constexpr unsigned computeHashAndFlags(unsigned hashBase, StringKind stringKind, BufferOwnership bufferOwnership, unsigned extraBits = 0)

Not thrilled with passing in "hashBase", which is effectively also an enumeration, as an untyped unsigned.

> Source/WTF/wtf/text/StringImpl.h:1240
> +        computeHashAndFlags(s_hashFlag8BitBuffer | s_hashFlagDidReportCost, stringKind, BufferInternal, (StringHasher::computeLiteralHashAndMaskTop8Bits(characters) << s_flagCount)), ConstructWithConstExpr)

Don’t need those parentheses around the last argument. Needed them before because they were part of an expression, but after a comma it’s not helpful, and in fact it makes it harder to read!

[David Kilzer (:ddkilzer)]

Comment on attachment 411606 [details]
Patch v1 redo

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

>> Source/WTF/wtf/text/StringImpl.h:213
>> +    static constexpr unsigned computeHashAndFlags(unsigned hashBase, StringKind stringKind, BufferOwnership bufferOwnership, unsigned extraBits = 0)
> 
> Not thrilled with passing in "hashBase", which is effectively also an enumeration, as an untyped unsigned.

The more I stare at `enum BufferOwnership` and `enum StringKind`, the more I realize that they're really all just StringAttributes, and probably should be a single enum with an OptionSet<>.

Will think about this some more before landing.

>> Source/WTF/wtf/text/StringImpl.h:1240
>> +        computeHashAndFlags(s_hashFlag8BitBuffer | s_hashFlagDidReportCost, stringKind, BufferInternal, (StringHasher::computeLiteralHashAndMaskTop8Bits(characters) << s_flagCount)), ConstructWithConstExpr)
> 
> Don’t need those parentheses around the last argument. Needed them before because they were part of an expression, but after a comma it’s not helpful, and in fact it makes it harder to read!

Will do.

[Radar WebKit Bug Importer]

<rdar://problem/70629961>