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

Mark more Uint128, Uint64 and Decimal operations as const #1157

Closed
wants to merge 1 commit into from

Conversation

archseer
Copy link

@archseer archseer commented Nov 6, 2021

I also tried making checked_ operations const (so I could make from_atomics const), but they return OverflowError which takes values via the ToString bound.

That seems like a of waste of gas, maybe we should type them OverflowError<T> and take the underlying value without calling to_string()? Better yet would be simply returning Option from checked_ operations which would line up with behavior in the standard library.

@webmaster128 webmaster128 mentioned this pull request Dec 15, 2021
11 tasks
@webmaster128
Copy link
Member

I also tried making checked_ operations const (so I could make from_atomics const), but they return OverflowError which takes values via the ToString bound.

That seems like a of waste of gas, maybe we should type them OverflowError<T> and take the underlying value without calling to_string()? Better yet would be simply returning Option from checked_ operations which would line up with behavior in the standard library.

This will be addressed in #1186. Thanks for bringing it up.

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Thank you for bringing this up. In #1186 I try to worganize things a bit sich that the API remains consistent across different types. There I added 3 lines to const is_zero/const fn atomics/const fn decimal_places. Would you be willing to address those in one PR per line such that all types receive the change?

@ueco-jb
Copy link
Contributor

ueco-jb commented Mar 21, 2022

@webmaster128 I'll take that over.

@webmaster128
Copy link
Member

Thanks a lot @archseer for bringing this up. Your work will be implemented as part of #1186. As you can see in the table there, we try to be very structured in order to ensure consistency across all types. So we'll go row by row.

If you see more APIs missing, please let us know in #1186.

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