Skip to content

ts_bitset: implement bit shift traits#155

Merged
danderson merged 1 commit intomainfrom
push-rokmkurllwwo
May 1, 2026
Merged

ts_bitset: implement bit shift traits#155
danderson merged 1 commit intomainfrom
push-rokmkurllwwo

Conversation

@danderson
Copy link
Copy Markdown
Member

@danderson danderson commented Apr 29, 2026

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_shl and unbounded_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 crate
  • wrapping_{shr,shl} has extremely confusing semantics and very limited real world utility
  • overflowing_{shr,shl} I could add easily, but we don't need it yet
  • checked_{shr,shl} ditto
  • rotate_{left,right} is in the same general family of bit ops, but we don't need it yet
  • funnel_{shr,shl} are experimental, and have weird SIMD-adjacent semantics we don't need yet

Happy to implement some/all of these to flesh out the shift ops if you think we should!

@danderson
Copy link
Copy Markdown
Member Author

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.

Copy link
Copy Markdown
Collaborator

@npry npry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Comment thread ts_bitset/src/lib.rs Outdated
@danderson
Copy link
Copy Markdown
Member Author

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.

@danderson danderson requested a review from npry April 30, 2026 03:18
Copy link
Copy Markdown
Collaborator

@npry npry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

other than the one note, looks good

Comment thread ts_bitset/src/lib.rs Outdated
@danderson
Copy link
Copy Markdown
Member Author

danderson commented Apr 30, 2026

Refactored to have base functions unbounded_{shl,shr}_inplace that operate on &mut self, then thin wrappers unbounded_{shl,shr} to match the std numeric API signature, and finally the macro implementations of the core::ops call the appropriate unbounded fn to do their thing.

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 foo = foor << bar properly. PTAL?

@danderson danderson requested a review from npry April 30, 2026 17:11
@danderson
Copy link
Copy Markdown
Member Author

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.

Comment thread ts_bitset/src/lib.rs Outdated
Updates #33

Signed-off-by: David Anderson <danderson@tailscale.com>
Change-Id: I8bfdf58ee33ba0486deb813474e3bbef6a6a6964
@danderson danderson force-pushed the push-rokmkurllwwo branch from 1d54b3a to f292e59 Compare May 1, 2026 01:57
@danderson danderson enabled auto-merge (rebase) May 1, 2026 02:02
@danderson danderson merged commit 0b8d819 into main May 1, 2026
25 checks passed
@danderson danderson deleted the push-rokmkurllwwo branch May 1, 2026 02:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants