Mask values on write instead of read

[bbaldino]

There have been at least a couple issues related to the fact that uX masks on read instead of write (i.e.: allows the underlying value of a uX value to be "invalid" for the advertised size of that type, where the "advertised" size is the X value in a uX). I've been meaning to take a look at masking values on write instead of read, such that a uX value's underlying type will always obey its advertised type and finally got around to it. The changes are actually much smaller than I was expecting, so that was a pleasant surprise. I'm putting this up as a draft because I'd like to try and add some more unit tests to validate the boundary cases (I haven't convinced myself that every one is covered yet).

I also wondered about adding debug asserts or even leveraging something like the contracts crate, but held off on that for now. Any thoughts @chrysn @Kijewski ?

[chrysn]

I haven't made up my mind on mask-before v. mask-after -- both have their merits (mask-before can be elided automatically when the value is already known to the compiler, mask-after allows the compiler to know that the high bits are zero, mask-after allows some maskings to be elided when performing wrapping operations between uX).

I'm open to using something like contracts. Not sure that precise crate is the right tool, because I'd hope that eventually, the accessors would do something equivalent to fn get(&self) -> $type { debug_assert!(self.0 < 128); unsafe { assume(self.0 < 128) }; self.0 }, where unsafe fn assume(cond: bool) { if !cond { unreachable_unchecked() } }, which gives the compiler usable information for later steps -- but let's do one thing at a time.

Consistency is certainly good, so my proposal here is as this: Let's go with this general direction, but please send the .0 access that used to be masked through an (initially private) .read(&self) -> $type function. That way we retain the flexibility to add debug asserts in the getter (get, read, happy with any color of the bike shed).

[bbaldino]

Consistency is certainly good, so my proposal here is as this: Let's go with this general direction, but please send the .0 access that used to be masked through an (initially private) .read(&self) -> $type function. That way we retain the flexibility to add debug asserts in the getter (get, read, happy with any color of the bike shed).

Sounds like a good idea, but is it worth waiting to the helper method until we're ready to do the validations? It'd be just as easy to add the helper when we want to add the validations as it is now, and doing so now just results in a do-nothing method. I also think we'll need read_inner(&self) -> $type and read_inner_mut(&mut self) -> &mut $type.

[chrysn]

Ah it's ready to merge ... let's just do it in a next PR.

[bbaldino]

@chrysn generally I think squashing commits when merging will help keep history cleaner as well