Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the private field crash for the Array object. #5139

Merged

Conversation

paintedveil5
Copy link
Contributor

Fix the private field crash in the Array object mentioned in
#5138

@zherczeg
Copy link
Member

zherczeg commented May 3, 2024

Good patch. Please add the testcase.

@paintedveil5
Copy link
Contributor Author

Good patch. Please add the testcase.

Added.

Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

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

LGTM

@zherczeg
Copy link
Member

zherczeg commented May 4, 2024

Please squash the commits and add DCO to the commit message.

@paintedveil5 paintedveil5 force-pushed the fix-private-field branch 2 times, most recently from c7dcf9a to 2c0f9d2 Compare May 7, 2024 02:45
Copy link
Member

@akosthekiss akosthekiss left a comment

Choose a reason for hiding this comment

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

The code seems OK, but the style of the test case addition is completely out of sync with the existing parts of the test case. Should be aligned.

@LaszloLango
Copy link
Contributor

@akosthekiss Please review the latest changes.

@ossy-szeged ossy-szeged added this to the Release 3.0.0 milestone Nov 12, 2024
@LaszloLango
Copy link
Contributor

We should check whether this fixes #5097 and #5100

@robertsipka
Copy link
Contributor

I've updated the test cases and rebased to the master.
This PR fixes #5097 and #5100 and #5138 as well.

@LaszloLango
Copy link
Contributor

@robertsipka could you please add new regression tests with the issue numbers for all three?

@robertsipka
Copy link
Contributor

robertsipka commented Nov 25, 2024

@robertsipka could you please add new regression tests with the issue numbers for all three?

I've followed the author's original intention and amended the other two test cases to the test case which was originally included in the PR, but It's actually easier to follow if they are added as regression tests, I modified it.

Fixes the following issues: 5097, 5100, 5138

Additional regression test cases added by: Robert Sipka <[email protected]>

JerryScript-DCO-1.0-Signed-off-by: Baihe Jiang <[email protected]>
JerryScript-DCO-1.0-Signed-off-by: Lily Jiang [email protected]
@LaszloLango LaszloLango dismissed akosthekiss’s stale review November 25, 2024 12:03

Requested change has been done.

@LaszloLango LaszloLango merged commit 95cc5e9 into jerryscript-project:master Nov 25, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants