Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2331303002:
Use ICU for ID_START and ID_CONTINUE for Unicode 9 data (Closed)
|
Created:
4 years, 3 months ago by jungshik at Google Modified:
3 years, 6 months ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/heads/master Project:
v8 Visibility:
Public. |
DescriptionUse ICU for ID_START, ID_CONTINUE and WhiteSpace check
Use ICU to check ID_Start, ID_Continue and WhiteSpace even for BMP
when V8_INTL_SUPPORT is on (which is default).
Change LineTerminator::Is() to check 4 code points from
ES#sec-line-terminators instead of using tables and Lookup function.
Remove Lowercase::Is(). It's not used anywhere.
Update webkit/{ToNumber,parseFloat}.js to have the correct expectation
for U+180E and the corresponding expected files. This is a follow-up to
an earlier change ( https://codereview.chromium.org/2720953003 ).
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_win_dbg,v8_mac_dbg;master.tryserver.chromium.android:android_arm64_dbg_recipe
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux_noi18n_rel_ng
BUG=v8:5370, v8:5155
TEST=unittests --gtest_filter=CharP*
TEST=webkit: ToNumber, parseFloat
TEST=test262: built-ins/Number/S9.3*, built-ins/parse{Int,Float}/S15*
TEST=test262: language/white-space/mong*
TEST=test262: built-ins/String/prototype/trim/u180e
TEST=mjsunit: whitespaces
Review-Url: https://codereview.chromium.org/2331303002
Cr-Commit-Position: refs/heads/master@{#45957}
Committed: https://chromium.googlesource.com/v8/v8/+/4aeb94a42d2abe7eb448aa416649ad7be5bdf3bc
Patch Set 1 #Patch Set 2 : fix a compile error #Patch Set 3 : fix link error #Patch Set 4 : fix more functions #Patch Set 5 : fix unittests #Patch Set 6 : fix more tests and update test status #Patch Set 7 : include LineTerminator tables in iI18n build #Patch Set 8 : fix more U+180E tests #Patch Set 9 : rebased #Patch Set 10 : V8_I18N => V8_INTL #Patch Set 11 : Add tests for Unicode 8/9 #Patch Set 12 : add a few more tests #Patch Set 13 : add more test for Unicode 8/9; fix multiline comments #Patch Set 14 : 1. Remove unused Lowercase::Is and related table 2. LineTerminator : drop the table and use 4 code points check #Patch Set 15 : Simplify WhiteSpaceOrLineTerminator::Is #Patch Set 16 : Do not mark U+180E-related tests as failing with no_i18n #Patch Set 17 : Speculative fix for linux bot failure #Patch Set 18 : undo the speculative fix to see if it's really necessary #
Total comments: 1
Patch Set 19 : fix a typo #Patch Set 20 : rebased #Patch Set 21 : Refers to PropList.txt for Other_ID_{Start,Continue} #
Total comments: 14
Patch Set 22 : review comments addressed #
Total comments: 1
Patch Set 23 : fix a typo in unicode.h #Patch Set 24 : drop an unnecessary todo #
Messages
Total messages: 100 (84 generated)
The CQ bit was checked by jshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/14095)
The CQ bit was checked by jshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/12582) v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
The CQ bit was checked by jshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/12601) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
The CQ bit was checked by jshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
The CQ bit was checked by jshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/8779) v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)
The CQ bit was checked by jshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Use ICU for ID_START and ID_CONTINUE BUG=... TEST=... ========== to ========== Use ICU for ID_START and ID_CONTINUE BUG=v8:5370 TEST=... ==========
The CQ bit was checked by jshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
The CQ bit was checked by jshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
The CQ bit was checked by jshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by jshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Use ICU for ID_START and ID_CONTINUE BUG=v8:5370 TEST=... ========== to ========== Use ICU for ID_START, ID_CONTINUE and WhiteSpace check Use ICU to check ID_Start, ID_Continue and WhiteSpace even for BMP when V8_INTL_SUPPORT is on (which is default). Change LineTerminator::Is() to check 4 code points from Ecma 262 2016 11.3 instead of using tables and Lookup function. Remove Lowercase::Is(). It's not used anywhere. BUG=v8:5370 TEST=unittests --gtest_filter=CharP* ==========
Description was changed from
==========
Use ICU for ID_START, ID_CONTINUE and WhiteSpace check
Use ICU to check ID_Start, ID_Continue and WhiteSpace even for BMP
when V8_INTL_SUPPORT is on (which is default).
Change LineTerminator::Is() to check 4 code points from Ecma 262 2016 11.3
instead of using tables and Lookup function.
Remove Lowercase::Is(). It's not used anywhere.
BUG=v8:5370
TEST=unittests --gtest_filter=CharP*
==========
to
==========
Use ICU for ID_START, ID_CONTINUE and WhiteSpace check
Use ICU to check ID_Start, ID_Continue and WhiteSpace even for BMP
when V8_INTL_SUPPORT is on (which is default).
Change LineTerminator::Is() to check 4 code points from Ecma 262 2016 11.3
instead of using tables and Lookup function.
Remove Lowercase::Is(). It's not used anywhere.
Update webkit/{ToNumber,parseFloat}.js to have the correct expectation
for U+180E and the corresponding expected files.
BUG=v8:5370
TEST=unittests --gtest_filter=CharP*
TEST=webkit: ToNumber, parseFloat
TEST=test262: built-ins/Number/S9.3*, built-ins/parse{Int,Float}/S15*
TEST=test262: language/white-space/mong*
TEST=test262: built-ins/String/prototype/trim/u180e
TEST=mjsunit: whitespaces
==========
The CQ bit was checked by jshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
Use ICU for ID_START, ID_CONTINUE and WhiteSpace check
Use ICU to check ID_Start, ID_Continue and WhiteSpace even for BMP
when V8_INTL_SUPPORT is on (which is default).
Change LineTerminator::Is() to check 4 code points from Ecma 262 2016 11.3
instead of using tables and Lookup function.
Remove Lowercase::Is(). It's not used anywhere.
Update webkit/{ToNumber,parseFloat}.js to have the correct expectation
for U+180E and the corresponding expected files.
BUG=v8:5370
TEST=unittests --gtest_filter=CharP*
TEST=webkit: ToNumber, parseFloat
TEST=test262: built-ins/Number/S9.3*, built-ins/parse{Int,Float}/S15*
TEST=test262: language/white-space/mong*
TEST=test262: built-ins/String/prototype/trim/u180e
TEST=mjsunit: whitespaces
==========
to
==========
Use ICU for ID_START, ID_CONTINUE and WhiteSpace check
Use ICU to check ID_Start, ID_Continue and WhiteSpace even for BMP
when V8_INTL_SUPPORT is on (which is default).
Change LineTerminator::Is() to check 4 code points from Ecma 262 2016 11.3
instead of using tables and Lookup function.
Remove Lowercase::Is(). It's not used anywhere.
Update webkit/{ToNumber,parseFloat}.js to have the correct expectation
for U+180E and the corresponding expected files.
BUG=v8:5370,v8:5155
TEST=unittests --gtest_filter=CharP*
TEST=webkit: ToNumber, parseFloat
TEST=test262: built-ins/Number/S9.3*, built-ins/parse{Int,Float}/S15*
TEST=test262: language/white-space/mong*
TEST=test262: built-ins/String/prototype/trim/u180e
TEST=mjsunit: whitespaces
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/25573) v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...) v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...) v8_linux64_verify_csa_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_arm64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng_trigg...) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_arm_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng_trigger...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/25569) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_linux_nodcheck_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng_tr...)
The CQ bit was checked by jshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jshin@chromium.org changed reviewers: + adamk@chromium.org, yangguo@chromium.org
Yang and Adam, PTAL Thanks Once icu_case_mapping flag is removed (which is already shipped), case-conversion-related mapping tables can be disabled from unicode.cc (i.e. by being enclosed by #ifndef V8_INTL_SUPPORT). Eventually, they can be removed once the minimal ICU can be always used (I will measure the size impact.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/25574) v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...) v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...) v8_linux64_verify_csa_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_arm64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng_trigg...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/25570) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
Description was changed from
==========
Use ICU for ID_START, ID_CONTINUE and WhiteSpace check
Use ICU to check ID_Start, ID_Continue and WhiteSpace even for BMP
when V8_INTL_SUPPORT is on (which is default).
Change LineTerminator::Is() to check 4 code points from Ecma 262 2016 11.3
instead of using tables and Lookup function.
Remove Lowercase::Is(). It's not used anywhere.
Update webkit/{ToNumber,parseFloat}.js to have the correct expectation
for U+180E and the corresponding expected files.
BUG=v8:5370,v8:5155
TEST=unittests --gtest_filter=CharP*
TEST=webkit: ToNumber, parseFloat
TEST=test262: built-ins/Number/S9.3*, built-ins/parse{Int,Float}/S15*
TEST=test262: language/white-space/mong*
TEST=test262: built-ins/String/prototype/trim/u180e
TEST=mjsunit: whitespaces
==========
to
==========
Use ICU for ID_START, ID_CONTINUE and WhiteSpace check
Use ICU to check ID_Start, ID_Continue and WhiteSpace even for BMP
when V8_INTL_SUPPORT is on (which is default).
Change LineTerminator::Is() to check 4 code points from Ecma 262 2016 11.3
instead of using tables and Lookup function.
Remove Lowercase::Is(). It's not used anywhere.
Update webkit/{ToNumber,parseFloat}.js to have the correct expectation
for U+180E and the corresponding expected files.
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_win_dbg,v8_mac_dbg;master.tryserver.chromium.android:android_arm64_dbg_recipe
BUG=v8:5370,v8:5155
TEST=unittests --gtest_filter=CharP*
TEST=webkit: ToNumber, parseFloat
TEST=test262: built-ins/Number/S9.3*, built-ins/parse{Int,Float}/S15*
TEST=test262: language/white-space/mong*
TEST=test262: built-ins/String/prototype/trim/u180e
TEST=mjsunit: whitespaces
==========
The CQ bit was checked by jshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from
==========
Use ICU for ID_START, ID_CONTINUE and WhiteSpace check
Use ICU to check ID_Start, ID_Continue and WhiteSpace even for BMP
when V8_INTL_SUPPORT is on (which is default).
Change LineTerminator::Is() to check 4 code points from Ecma 262 2016 11.3
instead of using tables and Lookup function.
Remove Lowercase::Is(). It's not used anywhere.
Update webkit/{ToNumber,parseFloat}.js to have the correct expectation
for U+180E and the corresponding expected files.
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_win_dbg,v8_mac_dbg;master.tryserver.chromium.android:android_arm64_dbg_recipe
BUG=v8:5370,v8:5155
TEST=unittests --gtest_filter=CharP*
TEST=webkit: ToNumber, parseFloat
TEST=test262: built-ins/Number/S9.3*, built-ins/parse{Int,Float}/S15*
TEST=test262: language/white-space/mong*
TEST=test262: built-ins/String/prototype/trim/u180e
TEST=mjsunit: whitespaces
==========
to
==========
Use ICU for ID_START, ID_CONTINUE and WhiteSpace check
Use ICU to check ID_Start, ID_Continue and WhiteSpace even for BMP
when V8_INTL_SUPPORT is on (which is default).
Change LineTerminator::Is() to check 4 code points from Ecma 262 2016 11.3
instead of using tables and Lookup function.
Remove Lowercase::Is(). It's not used anywhere.
Update webkit/{ToNumber,parseFloat}.js to have the correct expectation
for U+180E and the corresponding expected files.
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_win_dbg,v8_mac_dbg;master.tryserver.chromium.android:android_arm64_dbg_recipe
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux_noi18n_rel_ng
BUG=v8:5370,v8:5155
TEST=unittests --gtest_filter=CharP*
TEST=webkit: ToNumber, parseFloat
TEST=test262: built-ins/Number/S9.3*, built-ins/parse{Int,Float}/S15*
TEST=test262: language/white-space/mong*
TEST=test262: built-ins/String/prototype/trim/u180e
TEST=mjsunit: whitespaces
==========
The CQ bit was checked by jshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jshin@chromium.org changed reviewers: + littledan@chromium.org
mathias@qiwi.be changed reviewers: + mathias@qiwi.be
https://codereview.chromium.org/2331303002/diff/340001/src/char-predicates.h File src/char-predicates.h (right): https://codereview.chromium.org/2331303002/diff/340001/src/char-predicates.h#... src/char-predicates.h:39: // Non-BMP charaacters are not supported without I18N. typo: characters
On 2017/05/08 20:43:06, mathias wrote: > https://codereview.chromium.org/2331303002/diff/340001/src/char-predicates.h > File src/char-predicates.h (right): > > https://codereview.chromium.org/2331303002/diff/340001/src/char-predicates.h#... > src/char-predicates.h:39: // Non-BMP charaacters are not supported without I18N. > typo: characters Thanks. Fixed.
Ping, Dan, Adam and Yang :-) Thanks
The CQ bit was checked by jshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'd feel most comfortable with Dan reviewing this. But Dan, let me know if you don't have bandwidth and I can give it a more detailed look.
On 2017/06/08 19:55:39, adamk wrote: > I'd feel most comfortable with Dan reviewing this. But Dan, let me know if you > don't have bandwidth and I can give it a more detailed look. I'll review this patch by tomorrow.
Patch seems good to me, modulo stylistic suggestions below. Did you notice any performance change with this patch? E.g., Octane CodeLoad stresses parsing and may change performance with this patch. https://codereview.chromium.org/2331303002/diff/400001/src/char-predicates.cc File src/char-predicates.cc (right): https://codereview.chromium.org/2331303002/diff/400001/src/char-predicates.cc... src/char-predicates.cc:5: #ifdef V8_INTL_SUPPORT Nit: When an entire file is conditionally compiled for V8_INTL_SUPPORT, the new common practice across V8 is to do this in the build file, and then, within the file, check that the flag is enabled and error out if not. See src/intl.cc for an example. https://codereview.chromium.org/2331303002/diff/400001/src/char-predicates.cc... src/char-predicates.cc:27: } Great, this is how the code should look! Style nit: Did you consider using C character literals like '$'? Just curious, was this (c < 0x60) logic based on a benchmark? https://codereview.chromium.org/2331303002/diff/400001/src/char-predicates.cc... src/char-predicates.cc:29: // Ecma 262 7.0 11.2 White Space We do spec references these days in terms of anchors into the document, like // ES#sec-white-space White Space It'd be nice to have such a reference for IdentifierPart/IdentifierStart too. https://codereview.chromium.org/2331303002/diff/400001/src/char-predicates.cc... src/char-predicates.cc:30: // gC=Zs, U+0009, U+000B, U+000C, U+FEFF I like this comment--a similar one could make IdentifierStart/IdentifierPart easier to read too. https://codereview.chromium.org/2331303002/diff/400001/src/unicode.cc File src/unicode.cc (left): https://codereview.chromium.org/2331303002/diff/400001/src/unicode.cc#oldcode731 src/unicode.cc:731: } Seems like this code was simply dead all along? Good catch. https://codereview.chromium.org/2331303002/diff/400001/src/unicode.cc File src/unicode.cc (right): https://codereview.chromium.org/2331303002/diff/400001/src/unicode.cc#newcode... src/unicode.cc:1138: // Ecma 262 7.0 11.3 lists exactly 4 code points: Nit: Cite as ES#sec-line-terminators White Space https://codereview.chromium.org/2331303002/diff/400001/src/unicode.cc#newcode... src/unicode.cc:1146: // related tables with #ifndef V8_INTL_SUPPORT. Since icu_case_mapping has already shipped in Chrome, it should be fine to remove that flag at this point (in a separate patch). https://codereview.chromium.org/2331303002/diff/400001/test/webkit/ToNumber.js File test/webkit/ToNumber.js (right): https://codereview.chromium.org/2331303002/diff/400001/test/webkit/ToNumber.j... test/webkit/ToNumber.js:108: shouldBe("+mongolianVowelSeparator", "NaN"); This test expectations change seems appropriate to me, as a continuation of the work Yang started in https://codereview.chromium.org/2720953003 . Maybe you could reference this earlier partial work in the commit message when you're discussing this change.
Description was changed from
==========
Use ICU for ID_START, ID_CONTINUE and WhiteSpace check
Use ICU to check ID_Start, ID_Continue and WhiteSpace even for BMP
when V8_INTL_SUPPORT is on (which is default).
Change LineTerminator::Is() to check 4 code points from Ecma 262 2016 11.3
instead of using tables and Lookup function.
Remove Lowercase::Is(). It's not used anywhere.
Update webkit/{ToNumber,parseFloat}.js to have the correct expectation
for U+180E and the corresponding expected files.
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_win_dbg,v8_mac_dbg;master.tryserver.chromium.android:android_arm64_dbg_recipe
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux_noi18n_rel_ng
BUG=v8:5370,v8:5155
TEST=unittests --gtest_filter=CharP*
TEST=webkit: ToNumber, parseFloat
TEST=test262: built-ins/Number/S9.3*, built-ins/parse{Int,Float}/S15*
TEST=test262: language/white-space/mong*
TEST=test262: built-ins/String/prototype/trim/u180e
TEST=mjsunit: whitespaces
==========
to
==========
Use ICU for ID_START, ID_CONTINUE and WhiteSpace check
Use ICU to check ID_Start, ID_Continue and WhiteSpace even for BMP
when V8_INTL_SUPPORT is on (which is default).
Change LineTerminator::Is() to check 4 code points from Ecma 262 2016 11.3
instead of using tables and Lookup function.
Remove Lowercase::Is(). It's not used anywhere.
Update webkit/{ToNumber,parseFloat}.js to have the correct expectation
for U+180E and the corresponding expected files. This is a follow-up to
an earlier change ( https://codereview.chromium.org/2720953003 ).
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_win_dbg,v8_mac_dbg;master.tryserver.chromium.android:android_arm64_dbg_recipe
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux_noi18n_rel_ng
BUG=v8:5370,v8:5155
TEST=unittests --gtest_filter=CharP*
TEST=webkit: ToNumber, parseFloat
TEST=test262: built-ins/Number/S9.3*, built-ins/parse{Int,Float}/S15*
TEST=test262: language/white-space/mong*
TEST=test262: built-ins/String/prototype/trim/u180e
TEST=mjsunit: whitespaces
==========
Description was changed from
==========
Use ICU for ID_START, ID_CONTINUE and WhiteSpace check
Use ICU to check ID_Start, ID_Continue and WhiteSpace even for BMP
when V8_INTL_SUPPORT is on (which is default).
Change LineTerminator::Is() to check 4 code points from Ecma 262 2016 11.3
instead of using tables and Lookup function.
Remove Lowercase::Is(). It's not used anywhere.
Update webkit/{ToNumber,parseFloat}.js to have the correct expectation
for U+180E and the corresponding expected files. This is a follow-up to
an earlier change ( https://codereview.chromium.org/2720953003 ).
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_win_dbg,v8_mac_dbg;master.tryserver.chromium.android:android_arm64_dbg_recipe
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux_noi18n_rel_ng
BUG=v8:5370,v8:5155
TEST=unittests --gtest_filter=CharP*
TEST=webkit: ToNumber, parseFloat
TEST=test262: built-ins/Number/S9.3*, built-ins/parse{Int,Float}/S15*
TEST=test262: language/white-space/mong*
TEST=test262: built-ins/String/prototype/trim/u180e
TEST=mjsunit: whitespaces
==========
to
==========
Use ICU for ID_START, ID_CONTINUE and WhiteSpace check
Use ICU to check ID_Start, ID_Continue and WhiteSpace even for BMP
when V8_INTL_SUPPORT is on (which is default).
Change LineTerminator::Is() to check 4 code points from
Es#sec-line-terminators instead of using tables and Lookup function.
Remove Lowercase::Is(). It's not used anywhere.
Update webkit/{ToNumber,parseFloat}.js to have the correct expectation
for U+180E and the corresponding expected files. This is a follow-up to
an earlier change ( https://codereview.chromium.org/2720953003 ).
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_win_dbg,v8_mac_dbg;master.tryserver.chromium.android:android_arm64_dbg_recipe
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux_noi18n_rel_ng
BUG=v8:5370,v8:5155
TEST=unittests --gtest_filter=CharP*
TEST=webkit: ToNumber, parseFloat
TEST=test262: built-ins/Number/S9.3*, built-ins/parse{Int,Float}/S15*
TEST=test262: language/white-space/mong*
TEST=test262: built-ins/String/prototype/trim/u180e
TEST=mjsunit: whitespaces
==========
Description was changed from
==========
Use ICU for ID_START, ID_CONTINUE and WhiteSpace check
Use ICU to check ID_Start, ID_Continue and WhiteSpace even for BMP
when V8_INTL_SUPPORT is on (which is default).
Change LineTerminator::Is() to check 4 code points from
Es#sec-line-terminators instead of using tables and Lookup function.
Remove Lowercase::Is(). It's not used anywhere.
Update webkit/{ToNumber,parseFloat}.js to have the correct expectation
for U+180E and the corresponding expected files. This is a follow-up to
an earlier change ( https://codereview.chromium.org/2720953003 ).
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_win_dbg,v8_mac_dbg;master.tryserver.chromium.android:android_arm64_dbg_recipe
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux_noi18n_rel_ng
BUG=v8:5370,v8:5155
TEST=unittests --gtest_filter=CharP*
TEST=webkit: ToNumber, parseFloat
TEST=test262: built-ins/Number/S9.3*, built-ins/parse{Int,Float}/S15*
TEST=test262: language/white-space/mong*
TEST=test262: built-ins/String/prototype/trim/u180e
TEST=mjsunit: whitespaces
==========
to
==========
Use ICU for ID_START, ID_CONTINUE and WhiteSpace check
Use ICU to check ID_Start, ID_Continue and WhiteSpace even for BMP
when V8_INTL_SUPPORT is on (which is default).
Change LineTerminator::Is() to check 4 code points from
ES#sec-line-terminators instead of using tables and Lookup function.
Remove Lowercase::Is(). It's not used anywhere.
Update webkit/{ToNumber,parseFloat}.js to have the correct expectation
for U+180E and the corresponding expected files. This is a follow-up to
an earlier change ( https://codereview.chromium.org/2720953003 ).
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_win_dbg,v8_mac_dbg;master.tryserver.chromium.android:android_arm64_dbg_recipe
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux_noi18n_rel_ng
BUG=v8:5370,v8:5155
TEST=unittests --gtest_filter=CharP*
TEST=webkit: ToNumber, parseFloat
TEST=test262: built-ins/Number/S9.3*, built-ins/parse{Int,Float}/S15*
TEST=test262: language/white-space/mong*
TEST=test262: built-ins/String/prototype/trim/u180e
TEST=mjsunit: whitespaces
==========
> E.g., Octane CodeLoad stresses parsing and may change performance with this patch. Because the caching is still used, I'm not much worried about perf, but it'd be for sure a good idea to try benchmark. Sorry for my ignorance. How can I run Octane CodeLoad with a standalone v8 (rather than inside Chrome)? https://codereview.chromium.org/2331303002/diff/400001/src/char-predicates.cc File src/char-predicates.cc (right): https://codereview.chromium.org/2331303002/diff/400001/src/char-predicates.cc... src/char-predicates.cc:5: #ifdef V8_INTL_SUPPORT On 2017/06/10 06:00:12, Dan Ehrenberg wrote: > Nit: When an entire file is conditionally compiled for V8_INTL_SUPPORT, the new > common practice across V8 is to do this in the build file, and then, within the > file, check that the flag is enabled and error out if not. See src/intl.cc for > an example. Done. https://codereview.chromium.org/2331303002/diff/400001/src/char-predicates.cc... src/char-predicates.cc:27: } On 2017/06/10 06:00:12, Dan Ehrenberg wrote: > Great, this is how the code should look! > > Style nit: Did you consider using C character literals like '$'? Ok. Will use them. > Just curious, was this (c < 0x60) logic based on a benchmark? No. Perhaps, my attempt to optimize is not necessary. The result is cached and this function will not hit more than once for each character in most cases. What do you think? BTW, caching is another reason I'm not much worried about the perf. https://codereview.chromium.org/2331303002/diff/400001/src/char-predicates.cc... src/char-predicates.cc:29: // Ecma 262 7.0 11.2 White Space On 2017/06/10 06:00:12, Dan Ehrenberg wrote: > We do spec references these days in terms of anchors into the document, like > > // ES#sec-white-space White Space > > It'd be nice to have such a reference for IdentifierPart/IdentifierStart too. Done. https://codereview.chromium.org/2331303002/diff/400001/src/char-predicates.cc... src/char-predicates.cc:30: // gC=Zs, U+0009, U+000B, U+000C, U+FEFF On 2017/06/10 06:00:12, Dan Ehrenberg wrote: > I like this comment--a similar one could make IdentifierStart/IdentifierPart > easier to read too. Done. https://codereview.chromium.org/2331303002/diff/400001/src/unicode.cc File src/unicode.cc (right): https://codereview.chromium.org/2331303002/diff/400001/src/unicode.cc#newcode... src/unicode.cc:1146: // related tables with #ifndef V8_INTL_SUPPORT. On 2017/06/10 06:00:12, Dan Ehrenberg wrote: > Since icu_case_mapping has already shipped in Chrome, it should be fine to > remove that flag at this point (in a separate patch). Yup. that's my plan. https://codereview.chromium.org/2331303002/diff/400001/test/webkit/ToNumber.js File test/webkit/ToNumber.js (right): https://codereview.chromium.org/2331303002/diff/400001/test/webkit/ToNumber.j... test/webkit/ToNumber.js:108: shouldBe("+mongolianVowelSeparator", "NaN"); On 2017/06/10 06:00:12, Dan Ehrenberg wrote: > This test expectations change seems appropriate to me, as a continuation of the > work Yang started in https://codereview.chromium.org/2720953003 . Maybe you > could reference this earlier partial work in the commit message when you're > discussing this change. will do
The CQ bit was checked by jshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/13 09:37:30, jungshik at Google wrote: > > E.g., Octane CodeLoad stresses parsing and may change performance with this > patch. > > Because the caching is still used, I'm not much worried about perf, but it'd be > for sure a good idea to try benchmark. > Sorry for my ignorance. How can I run Octane CodeLoad with a standalone v8 > (rather than inside Chrome)? If you go to go/v8, you'll see explanation about how to use a Google-internal repository for this setup; I can't remember what it's called--qperf or something? We can also just land the patch and check out the automated performance measurement graphs, or... let the perf sheriffs yell at us if there's a regression. > > https://codereview.chromium.org/2331303002/diff/400001/src/char-predicates.cc > File src/char-predicates.cc (right): > > https://codereview.chromium.org/2331303002/diff/400001/src/char-predicates.cc... > src/char-predicates.cc:5: #ifdef V8_INTL_SUPPORT > On 2017/06/10 06:00:12, Dan Ehrenberg wrote: > > Nit: When an entire file is conditionally compiled for V8_INTL_SUPPORT, the > new > > common practice across V8 is to do this in the build file, and then, within > the > > file, check that the flag is enabled and error out if not. See src/intl.cc for > > an example. > > Done. > > https://codereview.chromium.org/2331303002/diff/400001/src/char-predicates.cc... > src/char-predicates.cc:27: } > On 2017/06/10 06:00:12, Dan Ehrenberg wrote: > > Great, this is how the code should look! > > > > Style nit: Did you consider using C character literals like '$'? > > Ok. Will use them. > > > Just curious, was this (c < 0x60) logic based on a benchmark? > > No. Perhaps, my attempt to optimize is not necessary. The result is cached and > this function will not hit more than once for each character in most cases. > What do you think? > > BTW, caching is another reason I'm not much worried about the perf. > > https://codereview.chromium.org/2331303002/diff/400001/src/char-predicates.cc... > src/char-predicates.cc:29: // Ecma 262 7.0 11.2 White Space > On 2017/06/10 06:00:12, Dan Ehrenberg wrote: > > We do spec references these days in terms of anchors into the document, like > > > > // ES#sec-white-space White Space > > > > It'd be nice to have such a reference for IdentifierPart/IdentifierStart too. > > Done. > > https://codereview.chromium.org/2331303002/diff/400001/src/char-predicates.cc... > src/char-predicates.cc:30: // gC=Zs, U+0009, U+000B, U+000C, U+FEFF > On 2017/06/10 06:00:12, Dan Ehrenberg wrote: > > I like this comment--a similar one could make IdentifierStart/IdentifierPart > > easier to read too. > > Done. > > https://codereview.chromium.org/2331303002/diff/400001/src/unicode.cc > File src/unicode.cc (right): > > https://codereview.chromium.org/2331303002/diff/400001/src/unicode.cc#newcode... > src/unicode.cc:1146: // related tables with #ifndef V8_INTL_SUPPORT. > On 2017/06/10 06:00:12, Dan Ehrenberg wrote: > > Since icu_case_mapping has already shipped in Chrome, it should be fine to > > remove that flag at this point (in a separate patch). > > Yup. that's my plan. > > https://codereview.chromium.org/2331303002/diff/400001/test/webkit/ToNumber.js > File test/webkit/ToNumber.js (right): > > https://codereview.chromium.org/2331303002/diff/400001/test/webkit/ToNumber.j... > test/webkit/ToNumber.js:108: shouldBe("+mongolianVowelSeparator", "NaN"); > On 2017/06/10 06:00:12, Dan Ehrenberg wrote: > > This test expectations change seems appropriate to me, as a continuation of > the > > work Yang started in https://codereview.chromium.org/2720953003 . Maybe you > > could reference this earlier partial work in the commit message when you're > > discussing this change. > > will do
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/13 10:18:36, Dan Ehrenberg wrote:
> On 2017/06/13 09:37:30, jungshik at Google wrote:
> > > E.g., Octane CodeLoad stresses parsing and may change performance with
this
> > patch.
> >
> > Because the caching is still used, I'm not much worried about perf, but it'd
> be
> > for sure a good idea to try benchmark.
> > Sorry for my ignorance. How can I run Octane CodeLoad with a standalone v8
> > (rather than inside Chrome)?
>
> If you go to go/v8, you'll see explanation about how to use a Google-internal
> repository for this setup; I can't remember what it's called--qperf or
> something?
>
> We can also just land the patch and check out the automated performance
> measurement graphs, or... let the perf sheriffs yell at us if there's a
> regression.
I ran Octane test locally. CodeLoad is neutral, but some other tests in Octane
are negative.
benchmark: score | idnstar | % |
===================================================+==========+========+
Richards: 25029.7 | 25231.5 | -0.8 |
DeltaBlue: 38120.5 | 37924.8 | 0.5 |
Crypto: 29724.7 | 29585.1 S 0.5 |
RayTrace: 57488.8 | 56967.3 S 0.9 |
EarleyBoyer: 38137.9 S 38392.5 | -0.7 |
RegExp: 5964.9 | 5923.6 | 0.7 |
Splay: 13618.3 S 14448.9 S -5.7 |
SplayLatency: 30099.2 S 31466.9 S -4.3 |
NavierStokes: 34570.9 | 34607.9 | -0.1 |
PdfJS: 22610.9 S 22298.2 S 1.4 |
Mandreel: 31624.4 | 31533.8 | 0.3 |
MandreelLatency: 68275.5 S 70795.9 S -3.6 |
Gameboy: 56633.7 S 56785.4 S -0.3 |
CodeLoad: 18546.8 S 18563.5 S |
Box2D: 47806.8 S 47062.3 S 1.6 |
zlib: 65571.3 | 65820.7 | -0.4 |
Typescript: 32921.6 | 32927.3 S |
Octane: 31164.3 S 31336.2 S -0.5 |
---------------------------------------------------+----------+--------+
On 2017/06/13 18:55:54, jungshik at Google wrote: > On 2017/06/13 10:18:36, Dan Ehrenberg wrote: > > On 2017/06/13 09:37:30, jungshik at Google wrote: > > > > E.g., Octane CodeLoad stresses parsing and may change performance with > this > > > patch. > > > > > > Because the caching is still used, I'm not much worried about perf, but it'd > > be > > > for sure a good idea to try benchmark. > > > Sorry for my ignorance. How can I run Octane CodeLoad with a standalone v8 > > > (rather than inside Chrome)? > > > > If you go to go/v8, you'll see explanation about how to use a Google-internal > > repository for this setup; I can't remember what it's called--qperf or > > something? > > > > We can also just land the patch and check out the automated performance > > measurement graphs, or... let the perf sheriffs yell at us if there's a > > regression. > > I ran Octane test locally. CodeLoad is neutral, but some other tests in Octane > are negative. > > benchmark: score | idnstar | % | > ===================================================+==========+========+ > Richards: 25029.7 | 25231.5 | -0.8 | > DeltaBlue: 38120.5 | 37924.8 | 0.5 | > Crypto: 29724.7 | 29585.1 S 0.5 | > RayTrace: 57488.8 | 56967.3 S 0.9 | > EarleyBoyer: 38137.9 S 38392.5 | -0.7 | > RegExp: 5964.9 | 5923.6 | 0.7 | > Splay: 13618.3 S 14448.9 S -5.7 | > SplayLatency: 30099.2 S 31466.9 S -4.3 | > NavierStokes: 34570.9 | 34607.9 | -0.1 | > PdfJS: 22610.9 S 22298.2 S 1.4 | > Mandreel: 31624.4 | 31533.8 | 0.3 | > MandreelLatency: 68275.5 S 70795.9 S -3.6 | > Gameboy: 56633.7 S 56785.4 S -0.3 | > CodeLoad: 18546.8 S 18563.5 S | > Box2D: 47806.8 S 47062.3 S 1.6 | > zlib: 65571.3 | 65820.7 | -0.4 | > Typescript: 32921.6 | 32927.3 S | > Octane: 31164.3 S 31336.2 S -0.5 | > ---------------------------------------------------+----------+--------+ Well, CodeLoad and TypeScript are the two that hit the parser the heaviest; if they're fine, and the other tests are sometimes slightly better, sometimes slightly worse, I'd guess that it could be run-to-run variation. Did you try running the suite a second time and see if the results are the same? lgtm % typo
https://codereview.chromium.org/2331303002/diff/420001/src/unicode.h File src/unicode.h (right): https://codereview.chromium.org/2331303002/diff/420001/src/unicode.h#newcode186 src/unicode.h:186: #ifndef V8_INTL_START V8_INTL_SUPPORT?
The CQ bit was checked by jshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/14 08:22:26, Dan Ehrenberg wrote: > https://codereview.chromium.org/2331303002/diff/420001/src/unicode.h > File src/unicode.h (right): > > https://codereview.chromium.org/2331303002/diff/420001/src/unicode.h#newcode186 > src/unicode.h:186: #ifndef V8_INTL_START > V8_INTL_SUPPORT? Good catch. Fixed in PS #23. As for the benchmark, I wish bsuite tools output stdev/sigma along with average (%-diff is not that meaningful especially when sigma is large. I wish to see diff in terms of sigma). Anyway, the number above is from 20 baseline runs compared with 20 test runs. I looked at raw scores and it swings quite a lot. I ran test (20 runs each) multiple times and Codeload is always within a 1-sigma of base runs while Typescript is sometimes slightly outside 1-sigma range of base. I tried to keep my desktop as 'quite' as possible but with Chrome running among other things (I didn't use it during benchmar run), it's likely that some jitters are due to external factors. I think I'd better land it and see what perf bots say.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jshin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from littledan@chromium.org Link to the patchset: https://codereview.chromium.org/2331303002/#ps460001 (title: "drop an unnecessary todo")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 460001, "attempt_start_ts": 1497468672612940,
"parent_rev": "8e646bd08cfb111c4f091c9de86564a5b602bda2", "commit_rev":
"4aeb94a42d2abe7eb448aa416649ad7be5bdf3bc"}
Message was sent while issue was closed.
Description was changed from
==========
Use ICU for ID_START, ID_CONTINUE and WhiteSpace check
Use ICU to check ID_Start, ID_Continue and WhiteSpace even for BMP
when V8_INTL_SUPPORT is on (which is default).
Change LineTerminator::Is() to check 4 code points from
ES#sec-line-terminators instead of using tables and Lookup function.
Remove Lowercase::Is(). It's not used anywhere.
Update webkit/{ToNumber,parseFloat}.js to have the correct expectation
for U+180E and the corresponding expected files. This is a follow-up to
an earlier change ( https://codereview.chromium.org/2720953003 ).
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_win_dbg,v8_mac_dbg;master.tryserver.chromium.android:android_arm64_dbg_recipe
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux_noi18n_rel_ng
BUG=v8:5370,v8:5155
TEST=unittests --gtest_filter=CharP*
TEST=webkit: ToNumber, parseFloat
TEST=test262: built-ins/Number/S9.3*, built-ins/parse{Int,Float}/S15*
TEST=test262: language/white-space/mong*
TEST=test262: built-ins/String/prototype/trim/u180e
TEST=mjsunit: whitespaces
==========
to
==========
Use ICU for ID_START, ID_CONTINUE and WhiteSpace check
Use ICU to check ID_Start, ID_Continue and WhiteSpace even for BMP
when V8_INTL_SUPPORT is on (which is default).
Change LineTerminator::Is() to check 4 code points from
ES#sec-line-terminators instead of using tables and Lookup function.
Remove Lowercase::Is(). It's not used anywhere.
Update webkit/{ToNumber,parseFloat}.js to have the correct expectation
for U+180E and the corresponding expected files. This is a follow-up to
an earlier change ( https://codereview.chromium.org/2720953003 ).
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_win_dbg,v8_mac_dbg;master.tryserver.chromium.android:android_arm64_dbg_recipe
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux_noi18n_rel_ng
BUG=v8:5370,v8:5155
TEST=unittests --gtest_filter=CharP*
TEST=webkit: ToNumber, parseFloat
TEST=test262: built-ins/Number/S9.3*, built-ins/parse{Int,Float}/S15*
TEST=test262: language/white-space/mong*
TEST=test262: built-ins/String/prototype/trim/u180e
TEST=mjsunit: whitespaces
Review-Url: https://codereview.chromium.org/2331303002
Cr-Commit-Position: refs/heads/master@{#45957}
Committed:
https://chromium.googlesource.com/v8/v8/+/4aeb94a42d2abe7eb448aa416649ad7be5b...
==========
Message was sent while issue was closed.
Committed patchset #24 (id:460001) as https://chromium.googlesource.com/v8/v8/+/4aeb94a42d2abe7eb448aa416649ad7be5b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
