Skip to content

Conversation

@ivandevp
Copy link
Contributor

Hey!

This PR adds Array.prototype.reverse method implementation.

There are still 3 tests failing but as far as I could check they're failing because some built-in constructors are still not implemented (e.g. TypedArrays), a bit of guidance on how to implement them would be highly apprecciated :)

@ivandevp ivandevp marked this pull request as ready for review October 16, 2024 13:39
@aapoalas
Copy link
Member

praise: Oh that's a beautiful fast path, absolutely splendid! Great work, and thank you! This looks good to merge from my point of view!

@ivandevp
Copy link
Contributor Author

praise: Oh that's a beautiful fast path, absolutely splendid! Great work, and thank you! This looks good to merge from my point of view!

Thanks for the fast follow up on this PR @aapoalas ❤️ - Just curious if we need to address the failing tests on this PR:

  "built-ins/Array/prototype/reverse/get_if_present_with_delete.js": "CRASH",
  "built-ins/Array/prototype/reverse/length-exceeding-integer-limit-with-proxy.js": "CRASH",
  "built-ins/Array/prototype/reverse/resizable-buffer.js": "CRASH",

If so, do you have any advice on how to fix them?

@aapoalas
Copy link
Member

praise: Oh that's a beautiful fast path, absolutely splendid! Great work, and thank you! This looks good to merge from my point of view!

Thanks for the fast follow up on this PR @aapoalas ❤️ - Just curious if we need to address the failing tests on this PR:

  "built-ins/Array/prototype/reverse/get_if_present_with_delete.js": "CRASH",
  "built-ins/Array/prototype/reverse/length-exceeding-integer-limit-with-proxy.js": "CRASH",
  "built-ins/Array/prototype/reverse/resizable-buffer.js": "CRASH",

If so, do you have any advice on how to fix them?

Ah yeah, sorry I'm traveling and your question kind of slipped my mind :)

No need to address them: They're failing for an entirely acceptable reason, at least the resizable buffer and proxy case. If the get_if_present_with_delete.js also is failing from eg. Proxy not being implemented then it's absolutely fine as is.

@ivandevp
Copy link
Contributor Author

Great - thanks for taking the time to check it @aapoalas - then yes, I think we can merge it as is for now 🚀

@aapoalas aapoalas merged commit f1e4e28 into trynova:main Oct 16, 2024
1 check passed
@aapoalas
Copy link
Member

Thank you again <3 Excellent code!

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.

2 participants