make CopyStringChars/CopyLinearStringChars accept start param, char* dest

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: myk, Assigned: myk)

Tracking

unspecified
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [v8api])

Attachments

(2 attachments)

SpiderShim uses custom implementations of CopyStringChars and CopyLinearStringChars [1] with two enhancements:

1. Per the V8 API, they accept an additional "start" parameter that identifies the index in the src string from which to start copying characters.

2. They're templatized, with char16_t* and char* specializations of CopyLinearStringChars, so SpiderShim's String::Write implementation can call CopyStringChars for both types of destination buffers [2].

It'd be useful to upstream these changes into SpiderMonkey, so SpiderShim can use the upstream versions.

[1] https://github.com/mozilla/spidernode/blob/e9b2841/deps/spidershim/src/v8string.h#L52-L108
[2] https://github.com/mozilla/spidernode/blob/e9b2841/deps/spidershim/src/v8string.cc#L363-L392
This patch implements the changes described in comment #0.  It passes the check-spidermonkey tests, although I don't think there are explicit tests for CopyStringChars.  It doesn't change CopyFlatStringChars, which isn't needed for implementing the V8 API, but perhaps it should, to keep the Copy*StringChars functions consistent.
Attachment #8750541 - Flags: review?(evilpies)
Comment on attachment 8750541 [details] [diff] [review]
add start param and char* specialization of CopyStringChars/CopyLinearStringChars

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

I don't really like the idea of an overload that implicitly truncates chars, but jsfriendapis are a lost cause.

::: js/src/jsfriendapi.h
@@ +887,2 @@
>  {
>      JS::AutoCheckCannotGC nogc;

We should be able to do MOZ_ASSERT(start + len <= GetLinearStringLength(s))

@@ +898,5 @@
> +
> +MOZ_ALWAYS_INLINE void
> +CopyLinearStringChars(char* dest, JSLinearString* s, size_t len, size_t start = 0)
> +{
> +    JS::AutoCheckCannotGC nogc;

Same assert.
Attachment #8750541 - Flags: review?(evilpies) → review+
(In reply to Tom Schuster [:evilpie] from comment #2)
> I don't really like the idea of an overload that implicitly truncates chars,
> but jsfriendapis are a lost cause.

FWIW, JS_EncodeStringToBuffer calls js::DeflateStringToBuffer, which does the same thing (although in that case it's the only option, so it truncates chars unconditionally).


> ::: js/src/jsfriendapi.h> We should be able to do MOZ_ASSERT(start + len <= GetLinearStringLength(s))> Same assert.

Good idea, I've added those asserts in this patch, which is what I'll push inbound.
https://hg.mozilla.org/mozilla-central/rev/87cb1c710305
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.