Jan de Mooij [:jandem] Assignee
	Description • 10 years ago

These are the more complicated functions, because they have to work with regular expressions and have all kinds of optimizations for flat strings and rope matching.

I'm going to handle the simple cases first and we'll see what's needed.

	Jan de Mooij [:jandem] Assignee
	Comment 1 • 10 years ago

Just enough changes to make the StringMatch case work.

	Jan de Mooij [:jandem] Assignee
	Comment 2 • 10 years ago

Uses rope matching if all leaf nodes have the same encoding. This made it pretty easy to convert the code to handle Latin1 ropes/patterns.

I verified the test calls RopeMatchImpl (and removing the all-leaf-nodes-have-same-encoding check triggers assertion failures).

	Luke Wagner [:luke]
	Comment 3 • 10 years ago

Comment on attachment 8433331 [details] [diff] [review]
Part 1 - StringMatch case

Review of attachment 8433331 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsstr.cpp
@@ +1827,5 @@
>  
>  /* Result of a successfully performed flat match. */
>  class FlatMatch
>  {
> +    RootedAtom pat;

Pre-existing, but can you append _ to pat?  Mixed use of _ looks tacky to me.

	Luke Wagner [:luke]
	Comment 4 • 10 years ago

Comment on attachment 8433469 [details] [diff] [review]
Part 2 - RopeMatch case

Review of attachment 8433469 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsstr.cpp
@@ +1398,4 @@
>              return false;
>          while (!r.empty()) {
> +            if (threshold-- == 0 ||
> +                r.front()->hasLatin1Chars() != text->hasLatin1Chars() ||

Might be worth it to hoist text->hasLatin1Chars(); I expect the compiler can't automatically.

	Jan de Mooij [:jandem] Assignee
	Comment 5 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3c9dcbc34c0
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2b79650ac7b

There's more to do here, but it depends on the regexp engine. Yarr will hopefully be gone next week so I'll wait for that to avoid breaking it.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 6 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/b3c9dcbc34c0
https://hg.mozilla.org/mozilla-central/rev/f2b79650ac7b

	Jan de Mooij [:jandem] Assignee
	Comment 7 • 10 years ago

This patch changes JSSubString from a (chars, length) struct to a (base, offset, length) struct. JSSubString isn't used across GC calls as far as I can see.

	Terrence Cole [:terrence]
	Comment 8 • 10 years ago

Comment on attachment 8440792 [details] [diff] [review]
Part 3 - Fix JSSubString

Review of attachment 8440792 [details] [diff] [review]:
-----------------------------------------------------------------

And now that it contains a raw JSString*, we'll hear about it from Hf if we ever do accidentally introduce GC while one is live. Nice!

	Jan de Mooij [:jandem] Assignee
	Comment 9 • 10 years ago

Part 3:

https://hg.mozilla.org/integration/mozilla-inbound/rev/69f9d7d919af

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 10 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/69f9d7d919af

	Jan de Mooij [:jandem] Assignee
	Comment 15 • 10 years ago

And of course there's an optimization for the case where the replacement is the empty string... So many paths str_replace can take, but I think I got all of them.

	Terrence Cole [:terrence]
	Comment 16 • 10 years ago

Comment on attachment 8442100 [details] [diff] [review]
Part 4 - FindDollarIndex

Review of attachment 8442100 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

	Terrence Cole [:terrence]
	Comment 17 • 10 years ago

Comment on attachment 8442101 [details] [diff] [review]
Part 5 - BuildDollarReplacement

Review of attachment 8442101 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/tests/latin1/replace.js
@@ +38,5 @@
> +    assertEq(s.replace(pat, "A$`\u1300"), "FooAFoo\u1300baz123");
> +    assertEq(s.replace(pat, "A$&\u1300"), "FooAbar\u1200\u1300baz123");
> +    assertEq(s.replace(pat, "A$'\u1300"), "FooAbaz123\u1300baz123");
> +}
> +testDollarReplacement();

Nice!

	Terrence Cole [:terrence]
	Comment 18 • 10 years ago

Comment on attachment 8442102 [details] [diff] [review]
Part 6 - RegExp replacement

Review of attachment 8442102 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

	Terrence Cole [:terrence]
	Comment 19 • 10 years ago

Comment on attachment 8442103 [details] [diff] [review]
Part 7 - flattenPattern

Review of attachment 8442103 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

	Terrence Cole [:terrence]
	Comment 20 • 10 years ago

Comment on attachment 8442104 [details] [diff] [review]
Part 8 - StrReplaceRegexpRemove

Review of attachment 8442104 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

	Jan de Mooij [:jandem] Assignee
	Comment 21 • 10 years ago

Thanks for the quick reviews!

Parts 4 and 5:

https://hg.mozilla.org/integration/mozilla-inbound/rev/95ce40562f51
https://hg.mozilla.org/integration/mozilla-inbound/rev/e589c195f61d

	Jan de Mooij [:jandem] Assignee
	Comment 22 • 10 years ago

And parts 6-8:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5358ae00940
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a8e50b64058
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc071decd42d

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 23 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/95ce40562f51
https://hg.mozilla.org/mozilla-central/rev/e589c195f61d
https://hg.mozilla.org/mozilla-central/rev/f5358ae00940
https://hg.mozilla.org/mozilla-central/rev/1a8e50b64058
https://hg.mozilla.org/mozilla-central/rev/fc071decd42d

	Hannes Verschore [:h4writer]
	Comment 24 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b7ede108faf

	Ryan VanderMeulen [:RyanVM]
	Comment 25 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/8b7ede108faf