PartialSumMap refactor #136
Conversation
| @@ -16,9 +16,18 @@ name = "wasmer_types" | |||
| [dependencies] | |||
| thiserror = "1.0" | |||
| indexmap = { version = "1.6" } | |||
| num-traits = "0.2.15" | |||
There was a problem hiding this comment.
My intuition here would be to “just” hard code the u32 key instead, over bringing in this dependency. OTOH, num-traits is already in the dep-graph so 🤷.
There was a problem hiding this comment.
My thoughts are, with num-traits we get the same benefits as with the sealed trait previously, aka being able to open-source PartialSumMap as a split crate easily :)
lib/types/src/partial_sum_map.rs
Outdated
| /// Get the current (virtual) size of this map. This is the sum of all `count` arguments passed to `push` until now. | ||
| /// | ||
| /// `O(1)` | ||
| pub fn size(&self) -> K { |
There was a problem hiding this comment.
I’m torn on the K return type here. Intuitively I’d expect this to return usize like the len functions for other datastructures do, even if this one isn’t really limited to usize elements. So probably:
| pub fn size(&self) -> K { | |
| pub fn len(&self) -> usize { |
There was a problem hiding this comment.
I've just pushed a commit documenting why I'm not returning usize here: basically, if K is a BigInt type, it could go overboard just by having a few really large intervals of BigInts.
And that's also the reason why I didn't pick len(), because len() is usually used with an usize return, and so I thought it'd be confusing. That said I feel much less strongly about this one, WDYT?
|
I've also pushed a commit removing the clone() in size(), but I'm much less sure about it (it basically forces our impl to keep a size member forever if we were to open-source it separately). WDYT? |
Fortunately this is not
Ah, well, I guess this will work out okay. |
Gud point 😅 |
As promised, the PartialSumMap refactor that gets rid of the sealed trait and the need for an explicit panic. Runtime drawback is one additional u32 being stored, which is negligible as it's not even a linear addition.
I also added a (bolero-based as we're not fuzzing this on nayduck yet anyway) fuzzer so that there's a good chance we detect regressions in this code just by running tests (bolero runs the fuzzer 1024 times by default when running tests)