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

refactor: use better Value and sub-enum names #68

Closed
wants to merge 1 commit into from

Conversation

sno2
Copy link
Contributor

@sno2 sno2 commented Nov 5, 2023

This should make a lot of our code easier to understand by displaying the number of bits in the name of our I56 type used to niche Number and BigInt. Furthermore, I made the niches start with the original type in the Value enum and their subtypes (Number and BigInt) which should also help with readability.

This should make a lot of our code easier to understand by displaying
the number of bits in the name of our `I56` type used to niche `Number`
and `BigInt`. Furthermore, I made the niches start with the original
type in the `Value` enum and their subtypes (`Number` and `BigInt`)
which should also help with readability.
@aapoalas
Copy link
Collaborator

aapoalas commented Nov 5, 2023

I'm not strongly in favour as I think that Integer is fairly self-explanatory, fits directly with the meaning of (Safe)Integers in 64-bit floating point numbers and is even often special cased in the spec.

Also if we do support eg. 16-bit architecture, as was pointed elsewhere, then these numbers will not be i56.

That being said, I have no great love for SmallBigInt nor Float.

@sno2
Copy link
Contributor Author

sno2 commented Nov 5, 2023

I'm not strongly in favour as I think that Integer is fairly self-explanatory, fits directly with the meaning of (Safe)Integers in 64-bit floating point numbers and is even often special cased in the spec.

Fair point, but our idea of SmallInteger is 56-bits, and I think that it may be a footgun to name it SmallInteger when we have to truncate it in most Number usages because we are supposed to only go up to 2^53. I56 explicitly makes the user understand how large the number type is and (hopefully) the reasoning behind it.

Also if we do support eg. 16-bit architecture, as was pointed elsewhere, then these numbers will not be i56.

To be frank, I think we could just disable such optimizations for these architectures.

@sno2 sno2 closed this Nov 5, 2023
@sno2
Copy link
Contributor Author

sno2 commented Nov 5, 2023

Yeah, it's probably fine in it's current state.

@sno2 sno2 deleted the refactor/rename-niches branch November 5, 2023 09:01
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