fix small_number() patch #379
Conversation
…rieve small atoms via small_number() even if they happen to be allocated on the heap, as normal atoms (but small).
Pull Request Test Coverage Report for Build 7870333649Details
💛 - Coveralls |
| ObjectType::Bytes => { | ||
| let atom = self.atom_vec[node.index() as usize]; | ||
| let buf = &self.u8_vec[atom.start as usize..atom.end as usize]; | ||
| if buf.len() > 3 || !canonical_positive_integer(buf) { |
There was a problem hiding this comment.
Why 3? Wouldn't it be okay to allow integers of size 4 that can still be represented by a u32?
There was a problem hiding this comment.
Oh wait, I get it now. You're checking if it CAN be representable as a u32 first.
There was a problem hiding this comment.
you're right that it would be possible to represent half of all 4-byte buffers as u32. This rule, however, is consistent with whether it can be represented as a SmallAtom node. Which is 26 bits (i.e. 3 bytes + 2 bits, we're rounding that to 3 bytes here).
This is the check in other places too, such as fn new_atom(&[u8]) -> NodePtr. Perhaps it would make sense to factor out this check to make sure it's consistent everywhere, and maybe it would even make sense to make it exactly 26 bits (rather than 24).
For instance, it just occurred to me that the value 0x800000 would not be considered fitting in a SmallAtom, even though it would, just because the leading zero byte would bump it to 4.
Anyway, the two approaches I see are:
- factor out this logic, make it more precise (to cover more large numbers)
- make this logic special, since we don't technically need it to fit in a
SmallAtom, we just need it to fit in au32
I'm leaning towards (1).
There was a problem hiding this comment.
I will propose a patch to improve this situation, aiming for (1). i.e. making this function behave consistently with whether an atom can be represented as a SmallAtom rather than whether it can be represented as a u32
The interpreter relies on being able to retrieve small atoms via small_number() even if they happen to be allocated on the heap, as normal atoms (but small).