Test case: /{1/u Expected: SyntaxError thrown Actual: RegExp instance ECMA262: https://tc39.es/ecma262/#prod-annexB-Term (/u flag precludes the use of ExtendedAtom) Term[U, N] :: [~U] ExtendedAtom[?N] ExtendedAtom[N] :: InvalidBracedQuantifier Test262: https://test262.report/browse/annexB/built-ins/RegExp/prototype/compile/pattern-string-invalid-u.js https://test262.report/browse/built-ins/RegExp/unicode_restricted_incomple_quantifier.js https://test262.report/browse/language/literals/regexp/u-invalid-extended-pattern-char.js
Created attachment 388739 [details] Patch
Comment on attachment 388739 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388739&action=review > Source/JavaScriptCore/yarr/YarrErrorCode.cpp:43 > + REGEXP_ERROR_PREFIX "incomplete {} quantifier for unicode pattern", // QuantifierIncomplete I think Unicode needs a capital "U" > Source/JavaScriptCore/yarr/YarrParser.h:817 > + if (m_isUnicode) { > + m_errorCode = ErrorCode::QuantifierIncomplete; > + break; > + } Seems like we need at least one test case that checks that we get the expected error. Maybe that’s not good in WPT tests because the actual error messages will be different across platforms, but if so then we could add a WebKit-specific test. Would you consider adding one? > Source/JavaScriptCore/yarr/YarrParser.h:-817 > + // If we did not find a complete quantifer, fall through to the default case. > + FALLTHROUGH; > } > - // if we did not find a complete quantifer, fall through to the default case. > - FALLTHROUGH; This change seems to make the code a tiny bit worse. Having the "FALLTHROUGH" macro inside a block doesn’t quite seem like the best style, and there’s no need to make this change except for stylistic reasons.
Created attachment 389191 [details] Patch Capitalize Unicode in error messages and add tests.
(In reply to Darin Adler from comment #2) Thank you for review(s), Darin. I always appreciate your detailed comments. > This change seems to make the code a tiny bit worse. Having the > "FALLTHROUGH" macro inside a block doesn’t quite seem like the best style, > and there’s no need to make this change except for stylistic reasons. According to my "research" (grep), "FALLTHROUGH" outside block is relatively popular in Source/WebCore (5 inside, 3 outside), but it is the only occurrence in Source/JavaScriptCore (24 inside, 1 of which is in this file).
Comment on attachment 389191 [details] Patch Rejecting attachment 389191 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 389191, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit /Volumes/Data/EWS/WebKit/JSTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: https://webkit-queues.webkit.org/results/13314000
Created attachment 389264 [details] Patch Set reviewer.
The commit-queue encountered the following flaky tests while processing attachment 389264 [details]: The commit-queue is continuing to process your patch.
The commit-queue encountered the following flaky tests while processing attachment 389264 [details]: fetch/fetch-worker-crash.html bug 187257 (author: youennf@gmail.com) The commit-queue is continuing to process your patch.
Comment on attachment 389264 [details] Patch Clearing flags on attachment: 389264 Committed r255452: <https://trac.webkit.org/changeset/255452>
All reviewed patches have been landed. Closing bug.
<rdar://problem/59039623>