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

Lightweight Snowflakes #717

Closed

Conversation

MrPowerGamerBR
Copy link
Contributor

@MrPowerGamerBR MrPowerGamerBR commented Nov 9, 2022

Changes Snowflakes to be value class to reduce their memory footprint.

I know that this PR probably won't be merged because it is a huge binary change, so it would break everything. Anyhow, this PR is here so other users can implement this change in their own Kord fork, if they need it.

Related to #711

Sadly the constructor can't have value.coerceIn(validValues), and we can't use a interface to "hide" the value class behind a SnowflakeImpl because, if you use interfaces, the class will be boxed. The failing tests are related to the missing coerceIn call.

@lukellmann
Copy link
Member

Sadly the constructor can't have value.coerceIn(validValues), and we can't use a interface to "hide" the value class behind a SnowflakeImpl because, if you use interfaces, the class will be boxed. The failing tests are related to the missing coerceIn call.

I thought about using require in init before, this approach would still work for a value class. It would still be a change in behavior though.

@ToxicMushroom
Copy link
Contributor

ToxicMushroom commented Nov 16, 2022

Can't we use a typealias for snowflake ?
It seems to just store a ULong and not do much else

@lukellmann
Copy link
Member

Can't we use a typealias for snowflake ?
It seems to just store a ULong and not do much else

That would mean that all extensions that we then would have to define on snowflakes are usable for all Longs...

@HopeBaron
Copy link
Member

This will be closed to be included in a up to date PR
Thanks @MrPowerGamerBR

@HopeBaron HopeBaron closed this Feb 16, 2023
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.

4 participants