search

cesanta / v7

Embedded JavaScript engine for C/C++
Other
1.43k stars 177 forks source link

Improved String.prototype.split() : added String-based support, fixed a couple of issues #510

Closed dimonomid closed 8 years ago

dimonomid commented 8 years ago

As the title suggests, if the separator given to String.prototype.split() is anything but RegExp, it is interpreted as String, and source string gets splitted by the plain string instead of RegExp. It works if v7 is built without RegExp support.

In addition, a couple of issues fixed, so that behavior of String.prototype.split() is now identical to that of browsers (Firefox, Chrome, Opera)

1:

Accordingly to spec, if pattern does not match an empty string, then the empty substring at the end of the source string should be included in the resulting array:

Accordingly to spec, ''.split(/.*/) should evaluate to [], not [ '' ], since given pattern matches an empty string.

Details in spec: http://www.ecma-international.org/ecma-262/5.1/#sec-15.5.4.14

3:

Non-ASCII strings were not handled correctly: for example, 'абв'.split(RegExp('')) should evaluate to ["а","б","в"], but previously it evaluated to ["�","�","�","�","�","�"]

4:

When the pattern contains capture groups, resulting array contained extra items:

Limit wasn't handled correctly. For example, '123'.split(/(x)*/, 2) should evaluate to ["1",undefined], but it evaluated to ["1",undefined,"2",undefined]

Review on Reviewable

mkmik commented 8 years ago

sorry, I didn't forget, I'll review this tomorrow.

dimonomid commented 8 years ago

Sergey asked to keep PRs containing just 1 commit, so, here we go: https://github.com/cesanta/v7/pull/512 . Closing this one.

mkmik commented 8 years ago

We don't mind (actually encourage) squashing and force pushing one's personal feature branch. So next time feel free to do that instead of creating a new PR.

On Mon, 19 Oct 2015, 23:22 Dmitry Frank notifications@github.com wrote:

Sergey asked to keep PRs containing just 1 commit, so, here we go: #512 https://github.com/cesanta/v7/pull/512 . Closing this one.

— Reply to this email directly or view it on GitHub https://github.com/cesanta/v7/pull/510#issuecomment-149351356.

dimonomid commented 8 years ago

Oh, ok, got it.