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

Implement variable sized bit array pattern matching on JavaScript #4273

Merged
merged 11 commits into from
Mar 4, 2025

Conversation

GearsDatapacks
Copy link
Member

This PR implements the ability to pattern match on dynamically sized bit array segments on JavaScript.

@GearsDatapacks GearsDatapacks force-pushed the variable-size-bit-slices branch from 7debf3d to 312c90b Compare February 21, 2025 19:08
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Wonderful, thank you!!!

I've left a bunch of little nit-pick notes inline, and it would be fab to remove all the clones.

I couldn't see any tests that involve extra, and there's no integration tests to check that that the runtime behaviour is correct. Here's an example bit array PR's integration tests: https://github.com/gleam-lang/gleam/pull/3946/files#diff-42aefabdf30f6176dcac76f43f95e6af038909579ce30683824c7fc2d98c3db9

Thanks again!

@lpil lpil marked this pull request as draft February 22, 2025 22:44
@GearsDatapacks GearsDatapacks force-pushed the variable-size-bit-slices branch from 9115deb to e62a1bb Compare February 23, 2025 00:35
@GearsDatapacks GearsDatapacks marked this pull request as ready for review February 23, 2025 11:16
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Wonderful work, as always! Thank you!!

@GearsDatapacks
Copy link
Member Author

What should I do about the linter errors? Should I use the shorthand *= and /= or should I disable that lint?

@lpil lpil enabled auto-merge (rebase) February 26, 2025 21:50
@GearsDatapacks
Copy link
Member Author

Ah, looks like tests are failing due to #3253. I guess I just have to work around that for now?

@lpil
Copy link
Member

lpil commented Feb 26, 2025

Reworking the test to avoid that bug could be good

auto-merge was automatically disabled February 26, 2025 21:59

Head branch was pushed to by a user without write access

@lpil lpil enabled auto-merge (rebase) February 26, 2025 22:00
@GearsDatapacks
Copy link
Member Author

Oops, forgot 928 doesn't fit within 8 bits

auto-merge was automatically disabled February 26, 2025 22:04

Head branch was pushed to by a user without write access

@GearsDatapacks
Copy link
Member Author

GearsDatapacks commented Feb 26, 2025

Ok sorry this has been a bit of a mess. Should be ready to merge now

@GearsDatapacks GearsDatapacks force-pushed the variable-size-bit-slices branch 2 times, most recently from 35ec68d to 1b7d1f3 Compare February 27, 2025 17:17
@GearsDatapacks
Copy link
Member Author

I've decided to follow your comment and replace the usage of Document in variables with EcoString. It makes it a bit easier to work with and removes most of the cloning. (The only cloning now is of EcoStrings, which is cheap)

@GearsDatapacks GearsDatapacks force-pushed the variable-size-bit-slices branch from e861c8c to 43b5974 Compare March 3, 2025 22:35
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Beautiful!! Thank you!

@lpil lpil merged commit 37b2008 into gleam-lang:main Mar 4, 2025
12 checks passed
@GearsDatapacks GearsDatapacks deleted the variable-size-bit-slices branch March 4, 2025 12:40
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