[regexp] implement latest spec draft for property class.

[regexp] implement latest spec draft for property class.

See https://github.com/mathiasbynens/es-regexp-unicode-property-escapes

Changes:
- only allow General Category, binary properties, Script, and Script_Extensions.
- implement Script_Extensions.

R=littledan@chromium.org
BUG=v8:4743
Committed: https://crrev.com/5beb5ee7e68e0e9925dcf49cf8626d7e82248bc6
Cr-Commit-Position: refs/heads/master@{#41091}

[Yang, 2016-11-15 13:06:44]

Description was changed from

==========
[regexp] implement latest spec draft.

See https://github.com/mathiasbynens/es-regexp-unicode-property-escapes

Changes:
- only allow General Category, binary properties, Script, and Script_Extensions.
- implement Script_Extensions.

R=littledan@chromium.org
BUG=v8:4743
==========

to

==========
[regexp] implement latest spec draft for property class.

See https://github.com/mathiasbynens/es-regexp-unicode-property-escapes

Changes:
- only allow General Category, binary properties, Script, and Script_Extensions.
- implement Script_Extensions.

R=littledan@chromium.org
BUG=v8:4743
==========

[Dan Ehrenberg, 2016-11-16 17:54:52]

lgtm

Script_Extensions sounds much more useful than Script. I think it's good to
stick with UTS 18 here in the shape of the feature, but maybe we should
publicize the fact that people should probably use scx= rather than sc= when we
ship the feature.

https://codereview.chromium.org/2502933002/diff/20001/src/regexp/regexp-parse...
File src/regexp/regexp-parser.cc (right):

https://codereview.chromium.org/2502933002/diff/20001/src/regexp/regexp-parse...
src/regexp/regexp-parser.cc:1207: }
You're removing more than support for blocks here. We might end up adding blocks
back in
(https://github.com/mathiasbynens/es-regexp-unicode-property-escapes/issues/18)
but probably won't add back in every single property ICU supports. Could you add
a test that checks that another random property name is not supported? e.g.,
NFKD_Quick_Check for an especially unlikely one.

https://codereview.chromium.org/2502933002/diff/20001/test/mjsunit/harmony/re...
File test/mjsunit/harmony/regexp-property-script-extensions.js (right):

https://codereview.chromium.org/2502933002/diff/20001/test/mjsunit/harmony/re...
test/mjsunit/harmony/regexp-property-script-extensions.js:10:
t(/\p{Script_Extensions=Bopo}+/u, "\u300A");
This test is really long. Some of the existing \p tests are already some of the
slowest in automation. Do we really benefit from having such a long test, rather
than choosing a couple representative lines?

[Yang, 2016-11-17 09:08:38]

On 2016/11/16 17:54:52, Dan Ehrenberg wrote:
> lgtm
> 
> Script_Extensions sounds much more useful than Script. I think it's good to
> stick with UTS 18 here in the shape of the feature, but maybe we should
> publicize the fact that people should probably use scx= rather than sc= when
we
> ship the feature.

Indeed.

> 
>
https://codereview.chromium.org/2502933002/diff/20001/src/regexp/regexp-parse...
> File src/regexp/regexp-parser.cc (right):
> 
>
https://codereview.chromium.org/2502933002/diff/20001/src/regexp/regexp-parse...
> src/regexp/regexp-parser.cc:1207: }
> You're removing more than support for blocks here. We might end up adding
blocks
> back in
>
(https://github.com/mathiasbynens/es-regexp-unicode-property-escapes/issues/18)
> but probably won't add back in every single property ICU supports. Could you
add
> a test that checks that another random property name is not supported? e.g.,
> NFKD_Quick_Check for an especially unlikely one.

I'll re-add these tests if blocks gets added. I also added a couple of others.

> 
>
https://codereview.chromium.org/2502933002/diff/20001/test/mjsunit/harmony/re...
> File test/mjsunit/harmony/regexp-property-script-extensions.js (right):
> 
>
https://codereview.chromium.org/2502933002/diff/20001/test/mjsunit/harmony/re...
> test/mjsunit/harmony/regexp-property-script-extensions.js:10:
> t(/\p{Script_Extensions=Bopo}+/u, "\u300A");
> This test is really long. Some of the existing \p tests are already some of
the
> slowest in automation. Do we really benefit from having such a long test,
rather
> than choosing a couple representative lines?

I was not entirely sure whether I'm using the ICU API the intended way here
(using "Script" as property to parse the property value, but then use
"Script_Extensions" to apply the property class). So I wanted to make extra sure
that we are doing the right thing. The entire test takes, in debug mode, less
than 400ms. Compared to other long-running tests, I'd say this is fine.

[Yang, 2016-11-17 09:08:43]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-11-17 09:08:44]

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/2502933002/#ps40001
(title: "update to 9.0.0")

[commit-bot: I haz the power, 2016-11-17 09:08:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 09:22:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-17 09:22:08]

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...)

[Dan Ehrenberg, 2016-11-17 09:24:53]

littledan@chromium.org changed reviewers:
+ jshin@chromium.org

[Dan Ehrenberg, 2016-11-17 09:25:21]

Jungshik, could you review the ICU API use?

[Yang, 2016-11-17 09:47:21]

The CQ bit was checked by yangguo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 09:47:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 10:02:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-17 10:02:38]

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...)

[Yang, 2016-11-17 10:25:19]

The CQ bit was checked by yangguo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 10:25:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 10:40:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-17 10:40:37]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)

[Yang, 2016-11-17 12:15:18]

The CQ bit was checked by yangguo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 12:15:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 12:40:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-17 12:40:42]

Dry run: This issue passed the CQ dry run.

[jungshik at Google, 2016-11-17 22:27:49]

LGTM

https://codereview.chromium.org/2502933002/diff/80001/src/regexp/regexp-parse...
File src/regexp/regexp-parser.cc (right):

https://codereview.chromium.org/2502933002/diff/80001/src/regexp/regexp-parse...
src/regexp/regexp-parser.cc:1088: // name lookup as if the property is Script.
Filed http://bugs.icu-project.org/trac/ticket/12856.

https://codereview.chromium.org/2502933002/diff/80001/src/regexp/regexp-parse...
src/regexp/regexp-parser.cc:1102: USet* set = uset_openEmpty();
Not for this CL, but I wonder if there is any reason to use C API here? virtuall
all uset_foo's are C wrappers over the corresponding C++ UnicodeSet API (in
common/unicode/uniset.h )

[Yang, 2016-11-18 06:07:13]

https://codereview.chromium.org/2502933002/diff/80001/src/regexp/regexp-parse...
File src/regexp/regexp-parser.cc (right):

https://codereview.chromium.org/2502933002/diff/80001/src/regexp/regexp-parse...
src/regexp/regexp-parser.cc:1088: // name lookup as if the property is Script.
On 2016/11/17 22:27:49, jungshik at google wrote:
> Filed http://bugs.icu-project.org/trac/ticket/12856. 

Thanks.

https://codereview.chromium.org/2502933002/diff/80001/src/regexp/regexp-parse...
src/regexp/regexp-parser.cc:1102: USet* set = uset_openEmpty();
On 2016/11/17 22:27:49, jungshik at google wrote:
> Not for this CL, but I wonder if there is any reason to use C API here?
virtuall
> all uset_foo's are C wrappers over the corresponding C++ UnicodeSet API (in
> common/unicode/uniset.h )

No reason. It's just the first one I came across. Maybe I should move them all
over to the C++ API.

[Yang, 2016-11-18 06:07:21]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-11-18 06:07:21]

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/2502933002/#ps80001
(title: "fix test")

[commit-bot: I haz the power, 2016-11-18 06:07:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-18 06:09:31]

Description was changed from

==========
[regexp] implement latest spec draft for property class.

See https://github.com/mathiasbynens/es-regexp-unicode-property-escapes

Changes:
- only allow General Category, binary properties, Script, and Script_Extensions.
- implement Script_Extensions.

R=littledan@chromium.org
BUG=v8:4743
==========

to

==========
[regexp] implement latest spec draft for property class.

See https://github.com/mathiasbynens/es-regexp-unicode-property-escapes

Changes:
- only allow General Category, binary properties, Script, and Script_Extensions.
- implement Script_Extensions.

R=littledan@chromium.org
BUG=v8:4743
==========

[commit-bot: I haz the power, 2016-11-18 06:09:31]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-18 06:10:22]

Description was changed from

==========
[regexp] implement latest spec draft for property class.

See https://github.com/mathiasbynens/es-regexp-unicode-property-escapes

Changes:
- only allow General Category, binary properties, Script, and Script_Extensions.
- implement Script_Extensions.

R=littledan@chromium.org
BUG=v8:4743
==========

to

==========
[regexp] implement latest spec draft for property class.

See https://github.com/mathiasbynens/es-regexp-unicode-property-escapes

Changes:
- only allow General Category, binary properties, Script, and Script_Extensions.
- implement Script_Extensions.

R=littledan@chromium.org
BUG=v8:4743

Committed: https://crrev.com/5beb5ee7e68e0e9925dcf49cf8626d7e82248bc6
Cr-Commit-Position: refs/heads/master@{#41091}
==========

[commit-bot: I haz the power, 2016-11-18 06:10:23]

Patchset 5 (id:??) landed as
https://crrev.com/5beb5ee7e68e0e9925dcf49cf8626d7e82248bc6
Cr-Commit-Position: refs/heads/master@{#41091}

[jungshik at Google, 2016-11-18 22:28:37]

On 2016/11/18 06:07:13, Yang wrote:
>
https://codereview.chromium.org/2502933002/diff/80001/src/regexp/regexp-parse...
> File src/regexp/regexp-parser.cc (right):
> 
>
https://codereview.chromium.org/2502933002/diff/80001/src/regexp/regexp-parse...
> src/regexp/regexp-parser.cc:1088: // name lookup as if the property is Script.
> On 2016/11/17 22:27:49, jungshik at google wrote:
> > Filed http://bugs.icu-project.org/trac/ticket/12856. 
> 
> Thanks.
> 
>
https://codereview.chromium.org/2502933002/diff/80001/src/regexp/regexp-parse...
> src/regexp/regexp-parser.cc:1102: USet* set = uset_openEmpty();
> On 2016/11/17 22:27:49, jungshik at google wrote:
> > Not for this CL, but I wonder if there is any reason to use C API here?
> virtuall
> > all uset_foo's are C wrappers over the corresponding C++ UnicodeSet API (in
> > common/unicode/uniset.h )
> 
> No reason. It's just the first one I came across. Maybe I should move them all
> over to the C++ API.

I see. In that case, moving over to C++ would be nice (a bit more performant in
this case and simpler/nicer in the code). 

FYI: http://icu-project.org/apiref/icu4c/ has a table listing major APIs in C
and C++. In some cases (like this one), C API is a wrapper over C++ API. For
others, the opposite is the case.