Bug 193101 - Create a different syntax for incorporating single characters in makeString
Summary: Create a different syntax for incorporating single characters in makeString
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Charlie Turner
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-01-03 03:02 PST by Charlie Turner
Modified: 2020-04-09 11:19 PDT (History)
7 users (show)

See Also:


Attachments
Patch (60.95 KB, patch)
2019-01-03 03:12 PST, Charlie Turner
no flags Details | Formatted Diff | Diff
Patch (65.20 KB, patch)
2019-01-03 03:38 PST, Charlie Turner
no flags Details | Formatted Diff | Diff
Patch (70.81 KB, patch)
2019-01-03 08:44 PST, Charlie Turner
no flags Details | Formatted Diff | Diff
Patch (70.72 KB, patch)
2019-01-03 11:21 PST, Charlie Turner
no flags Details | Formatted Diff | Diff
Patch (71.01 KB, patch)
2019-01-03 12:45 PST, Charlie Turner
no flags Details | Formatted Diff | Diff
Patch (68.23 KB, patch)
2019-01-13 15:05 PST, Charlie Turner
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews205 for win-future (13.00 MB, application/zip)
2019-01-13 17:37 PST, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Charlie Turner 2019-01-03 03:02:00 PST
Create a different syntax for incorporating single characters in makeString
Comment 1 Charlie Turner 2019-01-03 03:12:37 PST
Created attachment 358249 [details]
Patch
Comment 2 Charlie Turner 2019-01-03 03:14:48 PST
We had a test failure in the StringConcatenate_Unsigned fixture on WPE as described in https://bugs.webkit.org/show_bug.cgi?id=180720. Seems the best fix for this was to create a new syntax for raw characters rather than trying testing platform specific behaviour as we were before.
Comment 3 Charlie Turner 2019-01-03 03:38:06 PST
Created attachment 358250 [details]
Patch
Comment 4 EWS Watchlist 2019-01-03 03:40:11 PST
Attachment 358250 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/LiteralCharacter.h:62:  Do not use 'using namespace WTF::CharacterLiterals;'.  [build/using_namespace] [4]
ERROR: Source/WTF/wtf/text/LiteralCharacter.cpp:27:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 64 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Charlie Turner 2019-01-03 03:49:01 PST
(In reply to Build Bot from comment #4)
> Attachment 358250 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WTF/wtf/text/LiteralCharacter.h:62:  Do not use 'using
> namespace WTF::CharacterLiterals;'.  [build/using_namespace] [4]

Why not? ASCIILiteral.h uses exactly the same technique.


> ERROR: Source/WTF/wtf/text/LiteralCharacter.cpp:27:  Found other header
> before a header this file implements. Should be: config.h, primary header,
> blank line, and then alphabetically sorted.  [build/include_order] [4]

#include "config.h"
#include <wtf/text/LiteralCharacter.h>

Don't see what's wrong here.
Comment 6 Charlie Turner 2019-01-03 03:56:04 PST
Bummer, you can't have

LiteralCharacter<UChar> operator"" _ch(UChar ch);

When UChar == unsigned short according to https://en.cppreference.com/w/cpp/language/user_literal. Worked for me since it's char16_t here, which apparently is allowed. Hmm, for UChar then, we don't get to use a user literal.
Comment 7 Charlie Turner 2019-01-03 08:44:42 PST
Created attachment 358255 [details]
Patch
Comment 8 EWS Watchlist 2019-01-03 08:48:06 PST
Attachment 358255 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/LiteralCharacter.h:61:  Do not use 'using namespace WTF::CharacterLiterals;'.  [build/using_namespace] [4]
ERROR: Source/WebKit/UIProcess/glib/RemoteInspectorClient.cpp:156:  Missing spaces around :  [whitespace/init] [4]
ERROR: Source/WTF/wtf/text/LiteralCharacter.cpp:27:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 70 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Charlie Turner 2019-01-03 11:21:10 PST
Created attachment 358264 [details]
Patch
Comment 10 EWS Watchlist 2019-01-03 11:24:52 PST
Attachment 358264 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/LiteralCharacter.h:61:  Do not use 'using namespace WTF::CharacterLiterals;'.  [build/using_namespace] [4]
ERROR: Source/WebKit/UIProcess/glib/RemoteInspectorClient.cpp:156:  Missing spaces around :  [whitespace/init] [4]
ERROR: Source/WTF/wtf/text/LiteralCharacter.cpp:27:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 70 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Charlie Turner 2019-01-03 12:45:00 PST
Created attachment 358269 [details]
Patch
Comment 12 EWS Watchlist 2019-01-03 12:46:36 PST
Attachment 358269 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/LiteralCharacter.h:61:  Do not use 'using namespace WTF::CharacterLiterals;'.  [build/using_namespace] [4]
ERROR: Source/WebKit/UIProcess/glib/RemoteInspectorClient.cpp:156:  Missing spaces around :  [whitespace/init] [4]
ERROR: Source/WTF/wtf/text/LiteralCharacter.cpp:27:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 70 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Charlie Turner 2019-01-04 06:44:15 PST
So these remaining build failures are to do with not updating the relevant build scripts for mac and windows AFAICT. I'll wait for a review on the patch before making changes to the build scripts for those platforms.
Comment 14 Loïc Yhuel 2019-01-08 16:55:30 PST
(In reply to Charlie Turner from comment #2)
> We had a test failure in the StringConcatenate_Unsigned fixture on WPE as
> described in https://bugs.webkit.org/show_bug.cgi?id=180720. Seems the best
> fix for this was to create a new syntax for raw characters rather than
> trying testing platform specific behaviour as we were before.
I think the failure came from ICU using char16_t
So the current test could be fixed using :
#if PLATFORM(WIN) || U_ICU_VERSION_MAJOR_NUM >= 59


About your patch, is there any reason why LiteralCharacter<char> operator"" _ch(char ch) isn't inline ?
Everything else (LiteralCharacter, StringTypeAdapter, makeString) seems to be defined in the headers.
Comment 15 Charlie Turner 2019-01-09 01:29:06 PST
(In reply to Loïc Yhuel from comment #14)
> (In reply to Charlie Turner from comment #2)
> > We had a test failure in the StringConcatenate_Unsigned fixture on WPE as
> > described in https://bugs.webkit.org/show_bug.cgi?id=180720. Seems the best
> > fix for this was to create a new syntax for raw characters rather than
> > trying testing platform specific behaviour as we were before.
> I think the failure came from ICU using char16_t
> So the current test could be fixed using :
> #if PLATFORM(WIN) || U_ICU_VERSION_MAJOR_NUM >= 59

Yep, we can condition the test to pass, but I wanted to see what it would take to fix it properly. I received what I thought was a bug comment, but must have been private mail, explaining the root reason is an out of data set of ICU headers in WebKit. When I get time I'll look at upgrading them to version 59 headers, which should help our issue here.

For now, in the name of greenness, I'll go ahead an do a condition fix while I find time to fix it properly.

> 
> 
> About your patch, is there any reason why LiteralCharacter<char> operator""
> _ch(char ch) isn't inline ?
> Everything else (LiteralCharacter, StringTypeAdapter, makeString) seems to
> be defined in the headers.

It was to satisfy the ODR, but not reason why it can't be marked inline. I'll address that when I get round to the ICU headers upgrade.
Comment 16 Loïc Yhuel 2019-01-09 02:26:58 PST
If we trust Source/WTF/icu/README, the included headers should only be used on Mac.

At least WPE and GTK use system headers, which can be older than 59 (no minimum version enforced, not sure there would be a reason to, Ubuntu 16.04 has ICU 55.1 for example).
Comment 17 Charlie Turner 2019-01-10 10:04:54 PST
(In reply to Loïc Yhuel from comment #16)
> If we trust Source/WTF/icu/README, the included headers should only be used
> on Mac.
> 
> At least WPE and GTK use system headers, which can be older than 59 (no
> minimum version enforced, not sure there would be a reason to, Ubuntu 16.04
> has ICU 55.1 for example).

I've moved to temporarily guarding this away as suggested in https://bugs.webkit.org/show_bug.cgi?id=193319

Not sure what he conclusion is about whether the LiteralCharacter syntax is a worthwhile change for the future once the non-inlined UDO is fixed.
Comment 18 Charlie Turner 2019-01-10 12:59:21 PST
Workaround for now is proposed in https://bugs.webkit.org/show_bug.cgi?id=193332, but we need some kind of syntax to avoid the ambiguity described in that bug going forward.
Comment 19 Michael Catanzaro 2019-01-11 17:15:16 PST
Mmmm... I see the problem, but... nobody is going to remember to add the _ch suffix, and it won't be needed or useful unless you have older ICU, right? So only developers working with old ICU will notice. (That excludes anybody using our jhbuild moduleset.)

Almost seems better to drop support for older ICU as soon as we're able. Per the GTK/WPE dependencies policy, we can drop support for Ubuntu 16.04 and require modern ICU in April. Not sure when Apple will be able to do so. If it will be a while, then we've got a tough question here to figure out how to resolve this thorny issue. :(
Comment 20 Charlie Turner 2019-01-13 15:02:22 PST
(In reply to Michael Catanzaro from comment #19)
> Mmmm... I see the problem, but... nobody is going to remember to add the _ch
> suffix

You will get a compile errors in makeString if you don't use it.

> , and it won't be needed or useful unless you have older ICU, right?

Or if your platform (like Windows) defines it as wchar_t for instance.

> So only developers working with old ICU will notice. (That excludes anybody
> using our jhbuild moduleset.)
> 
> Almost seems better to drop support for older ICU as soon as we're able.

AFAIK, this is more a type-safety issue than an ICU upgrade, although the later exposed the problem. It's been mentioned elsewhere on bugzilla that we ought to have literal character syntax for this reason.
Comment 21 Charlie Turner 2019-01-13 15:05:44 PST
Created attachment 359008 [details]
Patch

Fix inlining issue
Comment 22 EWS Watchlist 2019-01-13 15:08:37 PST
Attachment 359008 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/LiteralCharacter.h:64:  Do not use 'using namespace WTF::CharacterLiterals;'.  [build/using_namespace] [4]
ERROR: Source/WebKit/UIProcess/glib/RemoteInspectorClient.cpp:156:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 2 in 68 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 Michael Catanzaro 2019-01-13 15:35:05 PST
I guess I'm convinced... let's see what other reviewers think.
Comment 24 EWS Watchlist 2019-01-13 17:37:07 PST
Comment on attachment 359008 [details]
Patch

Attachment 359008 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10737036

New failing tests:
http/tests/dom/set-document-location-host-to-accepted-values.html
http/tests/xmlhttprequest/workers/referer.html
fast/dom/DOMURL/parsing.html
http/tests/security/cross-origin-cached-scripts.html
http/tests/security/history-username-password.html
http/tests/security/location-href-clears-username-password.html
fast/dom/Element/hostname-host.html
http/tests/security/cross-origin-cached-scripts-parallel.html
http/tests/security/cross-origin-cached-images-with-memory-pressure.html
http/tests/cache/xhr-vary-header.html
http/tests/xmlhttprequest/origin-exact-matching.html
http/tests/security/cross-origin-cached-images.html
fast/dom/HTMLAnchorElement/anchor-element-href-parsing.html
http/tests/security/cross-origin-cached-images-parallel.html
http/tests/security/history-pushState-replaceState-from-sandboxed-iframe.html
Comment 25 EWS Watchlist 2019-01-13 17:37:19 PST
Created attachment 359010 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 26 Charlie Turner 2020-01-27 02:53:24 PST
Scope of this work is much larger than expected and this patch is going in the wrong direction is seems.
Comment 27 Darin Adler 2020-04-09 11:19:33 PDT
Now that we’ve updated UChar to be char16_t and not uint16_t on all platforms, the problem we were trying to fix here should be gone for all platforms!