Skip to content

Conversation

@Sympatron
Copy link
Collaborator

Fixes #121

/// Attempt to obtain a read grant. Returns `Ok((start, size))` on success.
fn read(&self) -> Result<(usize, usize), ReadGrantError>;

/// Attempt to obtain a split read grant. Return `Ok(((start1, size1), (start2, size2)))` on success.
Copy link
Owner

Choose a reason for hiding this comment

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

Do you think it's worth adding a default impl to this that always returns an error, in order to avoid a breaking change for now?

Copy link
Owner

Choose a reason for hiding this comment

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

If we DO make a breaking change, I wonder if it's worth making an struct OffsetSlice { offset: usize, len: usize }, and returning Result<[OffsetSlice; 2], ReadGrantError> or something in order to avoid complex tuple types (esp. since both are usizes types!)

Choose a reason for hiding this comment

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

Maybe instead of an error, always return an empty second section?
Though that might be more confusing in terms of debugging in some cases.

Copy link
Owner

Choose a reason for hiding this comment

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

I think that might be "too clever by half", yeah.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Another idea I just had would be to have a new trait SplitCoord: Coord that has split_read and split_release_inner (which I realized I need too). Then SplitGrantR can be given out only if Q::Coord :SplitCoord.
Adding a default impl would be fine, too. What do you prefer?
I like the OffsetSlice idea.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm open to the specialized trait idea, as long as it looks reasonable in practice! I know we do that with Notify already.

Copy link
Owner

Choose a reason for hiding this comment

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

That being said, I don't think having split grants adds any overhead, like having async notify would. That actually makes me think maybe we don't specialize it? Sorry to give you conflicting opinions, but I'd actually maybe save "specialization" traits for cases where having the specialized version "costs" more.

@jamesmunns
Copy link
Owner

I'm overall open to this, it would be nice to have some basic tests for this functionality (I know the new code isn't great in that regard yet).

@Sympatron
Copy link
Collaborator Author

Sympatron commented Jan 14, 2026

Since ReadGrantError is not #[non_exaustive] and does not have a fitting variant, I added a default implementation that just panics if you try to do a split read or release. So as it stands now it would be backwards compatible (I checked with cargo-semver-checks).
Since the split read/release functionality is kind of a superset of the normal read/release we could reduce code duplication by reusing the split read/release logic for the normal read/release. This would result in slightly bigger binary size if split reads are not used, but slightly smaller binary size if both are used.

When you are ready for breaking changes I would change a few things:

  • Use OffsetSlice instead of (usize, usize)
  • No more panic by default for split operations
  • Instead add a default implementation for the normal operation that uses the split methods


/// Mark `used1 + used2` bytes as available for writing.
/// `used1` corresponds to the first split grant, `used2` to the second.
fn split_release_inner(&self, _used1: usize, _used2: usize) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not super happy with this signature, but I can't put my finger on it...
What do you think?

@Sympatron Sympatron marked this pull request as ready for review January 14, 2026 22:30
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.

bbq2 does not support split reads

3 participants