search

dnsl48 / fraction

[Rust] Lossless fractions and decimals; drop-in float replacement
Apache License 2.0
83 stars 25 forks source link

Use match arm on tuple instead of nested if-else branches. #28

Closed hbina closed 4 years ago

hbina commented 4 years ago

What do you think using this instead of nested if-else?

dnsl48 commented 4 years ago

Thank you for the PR! That looks huge, I'll need some time to review it more carefully.

Here are a couple notes from the brief look of it:

  1. I'm hesitant about adding more non-core trait requirements to GenericInteger (talking about One and Zero here). It already has a lot and requiring more makes potential 3rd party implementations of this trait incompatible with the existing implementation. I wouldn't do that without convincing arguments, apart from mere refactoring.

  2. I'm not sure if there are any performance/memory effects from swapping match argument being a single ref to a couple (talking about match self vs match (self, other)). I'd like to have some simple benchmarking before we do this for readability’s sake