Bug 180622 - Use relaxed constexpr for StringHasher
Summary: Use relaxed constexpr for StringHasher
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-09 08:48 PST by Yusuke Suzuki
Modified: 2017-12-11 10:07 PST (History)
11 users (show)

See Also:


Attachments
Patch (14.23 KB, patch)
2017-12-09 08:49 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2017-12-09 08:48:49 PST
Use relaxed constexpr for StringHasher
Comment 1 Yusuke Suzuki 2017-12-09 08:49:39 PST
Created attachment 328910 [details]
Patch
Comment 2 JF Bastien 2017-12-09 11:50:35 PST
Comment on attachment 328910 [details]
Patch

Wooh! r=me
Comment 3 Yusuke Suzuki 2017-12-09 11:52:35 PST
Comment on attachment 328910 [details]
Patch

Yay!
Comment 4 WebKit Commit Bot 2017-12-09 12:12:00 PST
Comment on attachment 328910 [details]
Patch

Clearing flags on attachment: 328910

Committed r225726: <https://trac.webkit.org/changeset/225726>
Comment 5 WebKit Commit Bot 2017-12-09 12:12:01 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Radar WebKit Bug Importer 2017-12-09 12:12:57 PST
<rdar://problem/35953150>
Comment 7 Darin Adler 2017-12-09 14:48:24 PST
Comment on attachment 328910 [details]
Patch

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

> Source/WTF/wtf/text/StringHasher.h:250
> +    static constexpr UChar defaultConverter(char character)
>      {
>          return character;
>      }

I believe this is incorrect on systems where char is signed. A correct version would be one of these:

    return static_cast<unsigned char>(character);
    return static_cast<uint8_t>(character);
    return static_cast<LChar>(character);
Comment 8 Yusuke Suzuki 2017-12-10 04:54:29 PST
Comment on attachment 328910 [details]
Patch

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

>> Source/WTF/wtf/text/StringHasher.h:250
>>      }
> 
> I believe this is incorrect on systems where char is signed. A correct version would be one of these:
> 
>     return static_cast<unsigned char>(character);
>     return static_cast<uint8_t>(character);
>     return static_cast<LChar>(character);

Oh, nice. I think this does not cause a problem right now since it is only used for string literals: if a character's MSB is set, it is a leading character in UTF-8.
I'll land a follow-up patch.
Comment 9 Yusuke Suzuki 2017-12-10 05:00:31 PST
Committed r225730: <https://trac.webkit.org/changeset/225730>
Comment 10 JF Bastien 2017-12-11 09:40:12 PST
Looks like this or the follow-up causes build failures on some bots?


Step #3: ../../Source/WTF/wtf/text/StringHasher.h:252:28: error: class member cannot be redeclared
Step #3:     static constexpr UChar defaultConverter(char16_t character)
Step #3:                            ^
Step #3: ../../Source/WTF/wtf/text/StringHasher.h:237:28: note: previous definition is here
Step #3:     static constexpr UChar defaultConverter(UChar character)
Step #3:                            ^

https://oss-fuzz-build-logs.storage.googleapis.com/log-520a7b40-d87a-4f05-a85f-074a673871b9.txt
Comment 11 Yusuke Suzuki 2017-12-11 10:07:44 PST
(In reply to JF Bastien from comment #10)
> Looks like this or the follow-up causes build failures on some bots?
> 
> 
> Step #3: ../../Source/WTF/wtf/text/StringHasher.h:252:28: error: class
> member cannot be redeclared
> Step #3:     static constexpr UChar defaultConverter(char16_t character)
> Step #3:                            ^
> Step #3: ../../Source/WTF/wtf/text/StringHasher.h:237:28: note: previous
> definition is here
> Step #3:     static constexpr UChar defaultConverter(UChar character)
> Step #3:                            ^
> 
> https://oss-fuzz-build-logs.storage.googleapis.com/log-520a7b40-d87a-4f05-
> a85f-074a673871b9.txt

Nice, I've opened a bug for this. https://bugs.webkit.org/show_bug.cgi?id=180656