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

Simplify Rounding Modes #5411

Open
barakman opened this issue Dec 30, 2024 · 0 comments · May be fixed by #5412
Open

Simplify Rounding Modes #5411

barakman opened this issue Dec 30, 2024 · 0 comments · May be fixed by #5412

Comments

@barakman
Copy link
Contributor

barakman commented Dec 30, 2024

🧐 Motivation

  1. Remove unused API
  2. Extend existing API

📝 Details
The Rounding-Modes API (in the Math library) currently includes:

    enum Rounding {
        Floor, // Toward negative infinity
        Ceil, // Toward positive infinity
        Trunc, // Toward zero
        Expand // Away from zero
    }

The last two options are different from the first two options only when dealing with signed input.
However, throughout the codebase, rounding-modes are used strictly for unsigned input.
Hence as it stands, the last two options are redundant in the current version.

My initial motivation was adding a Nearest mode, which for non-negative (unsigned) input stands for:

  • Round down (towards zero) any value with remainder < 0.5
  • Round up (towards infinity) any value with remainder >= 0.5

Adding this new mode is questionable (and a lot more complicated) when the enum at hand is "seemingly" implemented to support both signed and unsigned input. By getting rid of what appears to be redundant (unused) API, it might make it easier to add this new mode. But even regardless of this new mode, it makes sense to clean-up the unused part of this API.

UPDATE:

Note that a more complete fix might actually be to get rid of the Rounding-Mode API altogether, and split each function which takes it as input into two functions - a floor function and a ceil function.

Reason being - it would save gas for the caller:

  • For the floor functions, checking the rounding mode AND calculating the additional info will be omitted
  • For the ceil functions, checking the rounding mode will be omitted

Of course, with 4 different rounding modes, this might "bloat" the Math library's code.
But if you choose to accept my proposal and reduce it to 2 different rounding modes, then taking it one step forward and splitting each one of the associated functions into a pair of functions might seem like an option for you to consider.

@barakman barakman linked a pull request Dec 30, 2024 that will close this issue
3 tasks
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 a pull request may close this issue.

1 participant