[Merged by Bors] - Added scale to unit for memory #407
Conversation
src/memory.rs
Outdated
| /// Scale up or down to the desired `binary_multiple`. Returns a new `Memory` and does | ||
| /// not change itself. | ||
| pub fn scale_to(&self, binary_multiple: BinaryMultiple) -> Self { | ||
| let self_discriminant = self.unit as i32; |
There was a problem hiding this comment.
I don't really love the approach of using the index of variants in the enum for the calculation. I'd much rather implement into for the enum explicitly and handle this in that code or something similar.
For example:
impl Into<i32> for CapacityUnit {
fn into(self) -> i32 {
match self {
CapacityUnit::Kibi => 1000,
CapacityUnit::Mebi => powi32(1000,2),
CapacityUnit::Gibi => powi32(1000,3),
CapacityUnit::Tebi => powi32(1000,4),
CapacityUnit::Pebi => powi32(1000,5),
CapacityUnit::Exbi => powi32(1000,6),
}
}
}There was a problem hiding this comment.
Im not sure what to do with that except calculate back the exponents? Would this be a compromise?
impl Into<i32> for BinaryMultiple {
fn into(self) -> i32 {
match self {
BinaryMultiple::Kibi => 0,
BinaryMultiple::Mebi => 1,
BinaryMultiple::Gibi => 2,
BinaryMultiple::Tebi => 3,
BinaryMultiple::Pebi => 4,
BinaryMultiple::Exbi => 5,
}
}
}
It fixes the exponent values (of the discriminats) from the enum and allows for simple calculation?
There was a problem hiding this comment.
You'd use it directly in line 129 of your pr, no?
Basically replace value: self.value * 1024f32.powi(diff), with value: self.value * (self_discriminant / to_discriminant), or similar.
But I am also happy to directly return the exponent.
There was a problem hiding this comment.
For JVM heap sizes you still have to multiply the value by some fraction. I feel, there is no need to expose the Memory struct to the outside world. A function that does what to_java_heap() does but in addition is ensures a given unit value should be enough. This function should of course just return a Result<u32, Error> since the caller knows the unit already.
I'm sorry I didn't implement the to_java_heap() better in the first place.
src/memory.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub fn value(&self) -> f32 { |
There was a problem hiding this comment.
As discussed, these getter don't look rusty.
There was a problem hiding this comment.
Yes ill remove them and add a to_java_heap() method directly on the Memory struct?
I think i still want to return Result<(u32, BinaryMultiple), Error> seperatly in case any product requires it?
|
bors merge |
## Description - added method for Memory to be scaled to a desired unit (required for ZooKeeper) Co-authored-by: Malte Sander <malte.sander.it@gmail.com>
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
Description
Review Checklist
Once the review is done, comment
bors r+(orbors merge) to merge. Further information