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

Add d128::quantize_with_mode function #40

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

haileys
Copy link
Contributor

@haileys haileys commented Nov 22, 2017

It would be useful to be able to be able to set the rounding mode to use in quantize. This pull request adds a new d128::quantize_with_mode function that allows exactly that.

@alkis
Copy link
Owner

alkis commented Nov 26, 2017

I think it is better if we make the context a bit more ergonomic and then this function won't be necessary. How about a public function with_mode that is a thin wrapper on top of with_context:

let result = d128::with_mode(MODE, | | x.quantize(y));

@haileys
Copy link
Contributor Author

haileys commented Nov 27, 2017

I actually think we should be very careful about exposing the context at all to end users.

With the exception of get_status and set_status, all the public functions on d128 are currently pure and entirely stateless.

I'm worried about the potential consequences of introducing dynamically scoped state (in the form of a with_mode function, or even a public with_context function) into d128.

This will allow for non-local effects - rather than being able to reason about arithmetic in isolation, the way arithmetic works will depend on the entire state of the call stack. If any callers of a function have called into d128::with_mode, that changes how that function works.

The subtle and hard-to-reason-about effects of introducing dynamically scoped state become even trickier when multiple crates use decimal arithmetic. If both crates rely on different versions of d128, they'll end up using different thread local contexts and everything is ok. On the other hand, if both crates rely on the same version, either crate can now affect how arithmetic in the other works. Changing a dependency of one crate should not affect how another unrelated crate works.

Introducing dynamically scoped state also forces the decimal crate to design around unwind safety. If the callback function to with_mode panics, the rounding mode will never be restored back to its original value. If the panic is caught and further computation is performed on the unwinding thread, decimal arithmetic could behave unexpectedly.

I think there are a lot of difficult, subtle issues that are introduced by exposing dynamically scoped state to end users. These issues will make using the decimal crate correctly harder and affect the ability for users to reason about arithmetic. For the reasons outlined above I'd strongly prefer that the functions that this crate provides remain mostly pure, rather than relying on any mutable state.

@alkis
Copy link
Owner

alkis commented Nov 27, 2017

I think with_mode gives you a lot of power and with it come some rough edges you need to think about. The nice thing about it is that the user can choose:

let result = d128::with_mode(MODE, | | x.quantize(y));

has no surprises. The following:

let result = d128::with_mode(MODE, | | calculate());

can be super powerful: the same computation can be done with different rounding modes.

If we go this route then we need to figure out how to make the rounding mode stick across library versions and also find a compromise for when code panics. For the latter I think it is ok to restore the context and let the unwinding thread run with the default.

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.

None yet

2 participants