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

These functions like "foo".bold() => "<b>foo</b>" are simple to self-host.

The attached patch does this, so that I don't have to refactor the C++ code to handle latin1 strings.

 2 files changed, 68 insertions(+), 177 deletions(-)

(And most of the added lines are JS instead of C++.)

These functions are not standardized, so I just followed the C++ implementation and V8's self-hosted implementation. I also added a jit-test and made sure it passes without the other changes.

	André Bargull [:anba]
	Comment 1 • 10 years ago

(In reply to Jan de Mooij [:jandem] from comment #0)
> These functions are not standardized, so I just followed the C++
> implementation and V8's self-hosted implementation. I also added a jit-test
> and made sure it passes without the other changes.

ES6 adds a specification for these functions: https://people.mozilla.org/~jorendorff/es6-draft.html#sec-additional-properties-of-the-string.prototype-object

(As far as I can see, ) The proposed changes will give a different result for:
---
String.prototype.fixed.call({toString: () => 1, valueOf: () => 2})
---

Before: "<tt>1</tt>"
After: "<tt>2</tt>" (V8 also returns this string)

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

Comment on attachment 8426391 [details] [diff] [review]
Patch

Thanks André, that's good to know. I'll update the patch.

There's also a difference in argument evaluation order. I was aware of that, but if these functions are being standardized we should fix it.

	André Bargull [:anba]
	Comment 4 • 10 years ago

Comment on attachment 8426457 [details] [diff] [review]
Patch

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

::: js/src/builtin/String.js
@@ +254,5 @@
> +    return "<sup>" + ToString(this) + "</sup>";
> +}
> +
> +function EscapeAttributeValue(v) {
> +    return ToString(v).replace(/"/g, "&quot;");

removeUnicodeExtensions() in builtin/Intl.js contains a note that directly calling String.prototype.replace with a RegExp object is not allowed in self-hosted code, because it sets alters the RegExp statics. Apart from that issue, shouldn't that line read `callFunction(std_String_replace, ...)`?

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

Good points again. I'm not very familiar with our self-hosting infrastructure.

Till, can you tell me what's the best way to do this?

	Till Schneidereit [:till]
	Comment 6 • 10 years ago

Comment on attachment 8426457 [details] [diff] [review]
Patch

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

Nice :)

One thing, though: as jorendorff recently discovered, ToString as defined in Utilities.js uses std_String for the coercion. That is valid, but wasteful, as it causes the entire String object and its prototype to be cloned over into the using compartment. Can you simply replace that implementation with `return v + ''`? That should have exactly the same result, but without the overhead.

::: js/src/builtin/String.js
@@ +254,5 @@
> +    return "<sup>" + ToString(this) + "</sup>";
> +}
> +
> +function EscapeAttributeValue(v) {
> +    return ToString(v).replace(/"/g, "&quot;");

Correct: this would need to use `callFunction`. However, as André also correctly says, .replace can't be used here at all.

The harder solution would be implement a .replace equivalent to the `regexp_exec_no_statics` intrinsic used in Intl.js. However, it shouldn't be needed here, really, as this should do the trick, too:


	var inputStr = ToString(v);
	var outputStr = '';
	var chunkStart = 0;
	for (var i = 0; i < inputStr.length; i++) {
		if (inputStr[i] === '"') {
			outputStr += callFunction(std_String_substring, inputStr, chunkStart, i);
			outputStr += inputStr.substring(chunkStart, i) + '&quot;';
			chunkStart = i + 1;
		}
	}
	if (chunkStart < inputStr.length) {
		outputStr += callFunction(std_String_substring, inputStr, chunkStart);
	}
	return outputStr;

(Also, you can either leave out the ToString here or in the callers. Maybe replace one with an assert.)

	André Bargull [:anba]
	Comment 7 • 10 years ago

(In reply to Till Schneidereit [:till] from comment #6)
> Can you simply replace that implementation with
> `return v + ''`? That should have exactly the same result, but without the
> overhead.

ToString(ToPrimitive(v)) is not the same as ToString(v), see comment #1 ;-)

	Till Schneidereit [:till]
	Comment 8 • 10 years ago

(In reply to André Bargull from comment #7)
> (In reply to Till Schneidereit [:till] from comment #6)
> > Can you simply replace that implementation with
> > `return v + ''`? That should have exactly the same result, but without the
> > overhead.
> 
> ToString(ToPrimitive(v)) is not the same as ToString(v), see comment #1 ;-)

Urgh. :(

Ok, I'll change ToString to an intrinsic tomorrow. We can land that before this patch and should be good on that front.

(In reply to Jan de Mooij [:jandem] from comment #5)
> Good points again. I'm not very familiar with our self-hosting
> infrastructure.
> 
> Till, can you tell me what's the best way to do this?

In general, I try to keep our self-hosting wiki page[1] up to date. Let me know if some important information is missing from that.

[1]: https://developer.mozilla.org/en-US/docs/SpiderMonkey/Internals/self-hosting

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/184fd695b135

Green Linux64 Try run: https://tbpl.mozilla.org/?tree=Try&rev=43c50ee89dc8

(In reply to Till Schneidereit [:till] from comment #6)
> The harder solution would be implement a .replace equivalent to the
> `regexp_exec_no_statics` intrinsic used in Intl.js. However, it shouldn't be
> needed here, really, as this should do the trick, too:

Great, that works nicely :)

(In reply to Till Schneidereit [:till] from comment #8)
> In general, I try to keep our self-hosting wiki page[1] up to date. Let me
> know if some important information is missing from that.
> 
> [1]:
> https://developer.mozilla.org/en-US/docs/SpiderMonkey/Internals/self-hosting

Thanks, I'll take a look.

	Carsten Book [:Tomcat]
	Comment 10 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/184fd695b135