Make the Monty::new borrow the Uint
#600
Conversation
|
@fjarri I'm guessing this will need changes in |
| fn new(value: Self::Integer, params: Self::Params) -> Self { | ||
| BoxedMontyForm::new(value, params) | ||
| fn new(value: &Self::Integer, params: Self::Params) -> Self { | ||
| BoxedMontyForm::new(value.clone(), params) |
There was a problem hiding this comment.
Hmm, I know I already hit approve but this has me second guessing if it's worth it. MontyForm is a Copy type but BoxedMontyForm is a heap-backed type and this is adding another heap allocation which could otherwise be avoided.
There was a problem hiding this comment.
Yeah my heart sank a bit too when I had to do this. Then I figured that 1) a Box is cheap to clone and 2) the unboxed variant is probably what users that care about performance would prefer, so perhaps it makes sense to accommodate them?
There was a problem hiding this comment.
The heap allocation is a tangible performance hit, whereas passing a Copy type by reference may have no performance impact, particularly with inlining
There was a problem hiding this comment.
As a user of the API before this PR I find that I end up having to clone my Integers more often than I'd like, given that MontyForm just reads the data. But people instantiating with BoxedMontyForm will need the same clones in their code yeah? It's less annoying for them I guess, because if they read the code they see that the value is actually mutated in place.
At the end of the day I think it comes down to what the API is trying to communicate. Taking ownership of the arguments signals that the users aren't supposed to re-use the values, but when it comes to "number types" I think the expectation is that you can actually continue using it (which is the case for Copy types of course).
Maybe this is tricky to decide because the underlying impls are so different. Would it make sense to try to refactor BoxedMontyForm::new to take a ref and then do the minimum requried amount of copying?
There was a problem hiding this comment.
Why are you cloning a Copy type?
There was a problem hiding this comment.
Because I'm using it in generic code, through the Integer trait (and the Monty associated type) so I don't know if my users are going to use it with MontyForm or BoxedMontyForm.
|
My personal approach is to take by reference the arguments that are going to be processed in some way, and by value the ones that are going to be used verbatim (e.g. saved in a struct). So for this method I would vote for passing the argument by reference. But in the boxed case, while the value of the integer is not used verbatim, the heap memory where it's stored is. I can see the value of that to avoid an allocation. Would it be useful to have two |
|
I quite like having two separate initializers, reflecting more explicitly what is going on under the hood. |
|
Two initializers where one just calls (When I made my original comment about changing this to a reference, I guess I forgot that |
|
Fair enough! Thank you for taking the time to hash it out. :) |
Change the
Montytrait'snewmethod to take a ref to theIntegerinstead of ownership.Fixes #599