Skip to content

Conversation

@gergocs
Copy link
Contributor

@gergocs gergocs commented Nov 25, 2024

This patch fixes #4888.

The implementation is based on PR #4898, only resolved the conflicts and
applied requested changes.

Co-authored-by: Robert Fancsik robert.fancsik@h-lab.eu
JerryScript-DCO-1.0-Signed-off-by: Gergo Csizi gergocs@inf.u-szeged.hu

@ossy-szeged ossy-szeged added this to the Release 3.0.0 milestone Nov 25, 2024

memcpy (dst_buffer_p, src_buffer_p, count << info_p->shift);
}
else if (count >= new_typedarray_info.offset)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be fair, there was a comment on the original PR which has not been answered yet. I paste it here.

What if count is less than the target offset? I don't think this condition is correct.

var buf = new ArrayBuffer(10);
var a1 = new Int8Array(buf, 0, 5);
a1.fill(1);
a1.constructor = {
    [Symbol.species]: function (len) {
        return new Int8Array(buf, 5, 5);
    }
};
var a2 = a1.slice(2,4);
res = new Int8Array(buf, 0, 10);

Also, I don't think checking the offset is the right way, we need to check whether the two typedarrays use the same arraybuffer instead, and only then fall back to copying element-by-element.

Originally posted by @dbatyai in #4898 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the example and it worked just fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks.

Copy link
Contributor

@LaszloLango LaszloLango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@robertsipka robertsipka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have concerns about the results. The test case linked by Dániel run without an error message, but the result is correct? Sadly, as I see, no, please check it, use the mentioned test case in the PR as well, and make a results validation for res in the last line. Dániel's review seems appropriate to me.

This patch fixes jerryscript-project#4888.

The implementation is based on PR jerryscript-project#4898, only resolved the conflicts and
applied requested changes.

Co-authored-by: Robert Fancsik robert.fancsik@h-lab.eu
JerryScript-DCO-1.0-Signed-off-by: Gergo Csizi gergocs@inf.u-szeged.hu
Copy link
Contributor

@robertsipka robertsipka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@LaszloLango LaszloLango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still LGTM

@LaszloLango LaszloLango merged commit ba89576 into jerryscript-project:master Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants