Conversation
|
One more note: this is my first use of macros in anger. I mimicked what I saw in std's implementation of these ops, so hopefully I'm not too far afield, but I'm very interested in learning what I did wrong. In particular, I'm not loving the amount of checked casts between numeric types (any to usize, then usize to u32), but the shapes of the various APIs and the wide range of widths that usize can take seem to force my hand. My hope is that the compiler should be sufficiently smart to coalesce and/or elide many of those checked casts when monomorphizing, but I welcome a better way if you see one. |
173e3e0 to
8e5b3ec
Compare
npry
left a comment
There was a problem hiding this comment.
lgtm, i'm following your logic on the casting and agree it seems necessary, can't get around the fact that you need to use a usize for array index
macros look good. informationally, the other approach i'd consider here would be num_traits::ToPrimitive, but I think the macro is better than the extra dep
nerd-snipe nit: might be nice to implement the base unbounded_sh{l,r} as in-place mutations to avoid sticking another large [u64] array on the stack and then copying it back out when not required (e.g. in Sh{l,r}Assign). truly no pressure, i think the copy will be totally fine practically speaking, just if you're so inclined :)
8e5b3ec to
eedc465
Compare
|
Good call, I rewrote the unbounded shifts to work in-place on self, PTAL? The main shift loops end up a little more awkward due to having to dodge the borrow checker (can't do a mutable iteration while reading from other parts of the array), but it's not too different. |
eedc465 to
ca52041
Compare
npry
left a comment
There was a problem hiding this comment.
other than the one note, looks good
ca52041 to
1d54b3a
Compare
|
Refactored to have base functions We still end up relying on hoping that the compiler will optimize out copies if you use the non-assigning operations, as you'd expect. But the ops that are expected to be in-place are now explicitly so, so fingers crossed that'll help the compiler optimize things like |
|
Oh: I did end up making the in-place unbounded fns public as well. This doesn't match the std numeric APIs, but I decided it was reasonable to expose both the assigning and non-assigning variants of the unbounded op, to mirror the two flavors of bounded ops that the core::ops traits provide. |
Updates #33 Signed-off-by: David Anderson <danderson@tailscale.com> Change-Id: I8bfdf58ee33ba0486deb813474e3bbef6a6a6964
1d54b3a to
f292e59
Compare
Updates #33
I chose to mimic the semantics of the std shift APIs: the core::ops traits are implemented for shift amounts of all numeric types, signed and unsigned. Negative shifts panic, as do shifts of amounts greater or equal to the number of bits in the bitset.
Separately, I implemented
unbounded_shlandunbounded_shr, with the same signature as the primitive numeric types, which allows shifts greater than the bitset size. This is the operation I actually need in ts_tunnel, where the bitset is tracking a window of recently seen packet IDs that may shift by unbounded amounts according to the whims of the network.I didn't implement the other shift variants, basically because I don't think we need them yet:
unchecked_{shr,shl}would need unsafe, is forbidden in the cratewrapping_{shr,shl}has extremely confusing semantics and very limited real world utilityoverflowing_{shr,shl}I could add easily, but we don't need it yetchecked_{shr,shl}dittorotate_{left,right}is in the same general family of bit ops, but we don't need it yetfunnel_{shr,shl}are experimental, and have weird SIMD-adjacent semantics we don't need yetHappy to implement some/all of these to flesh out the shift ops if you think we should!