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

Decreased Enum Size by Boxing, Changed Map to use HashMap internally, Added ByteList for STRING_EXT #8

Merged
merged 3 commits into from
Sep 23, 2023

Conversation

Aderinom
Copy link
Contributor

Hello Takeru,

I'm using this library in a prototype of mine.
There were some QOL things I wanted to change for myself, which is why I'm offering this PR.

All of these are Breaking Changes!

The changes include:

  1. Boxing the Large Terms like Reference, ExternalFun, InternalFun to decrease the size of the enum from ~152bit to ~64bit
  2. Changing the Map to actually use a HashMap, simplifying it's use.
  3. Adding ByteList to handle STRING_EXT
    -> This can cause issues when a system happens to send a list of numbers which could but are not neccesarily under u8 as this would cause matches which expect a ByteList to fail.

I'll leave it up to you if you are interested in these changes.

Best Regards!

@sile
Copy link
Owner

sile commented Sep 22, 2023

Thank you! I'll review this PR tomorrow.

@sile
Copy link
Owner

sile commented Sep 23, 2023

The changes you proposed in this PR sounds good to me 👍

Copy link
Owner

@sile sile left a comment

Choose a reason for hiding this comment

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

LGTM

@sile sile merged commit 21c83b4 into sile:master Sep 23, 2023
@michalmuskala
Copy link

This change made it much more contrived to use the pattern API to extract strings. Specifically, now pattern::Str does not match if the data underneath produces Term::ByteList

@sile
Copy link
Owner

sile commented Mar 27, 2024

@michalmuskala Thank you for your comment. Would you be able to create an issue or pull request (if you already have an idea to fix that) to further discuss the topic?

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.

3 participants