Skip to content

Added split_read() function to be able to read all commited data at once#77

Merged
jamesmunns merged 10 commits intojamesmunns:masterfrom
Sympatron:master
Nov 12, 2020
Merged

Added split_read() function to be able to read all commited data at once#77
jamesmunns merged 10 commits intojamesmunns:masterfrom
Sympatron:master

Conversation

@Sympatron
Copy link
Collaborator

@Sympatron Sympatron commented Oct 23, 2020

Went ahead and implemented the idea I had in #76 myself.
There are 3 is 1 TODOs left:
1/2: I wrapped the second buffer in an Option, but that means I can't easily return it as Option<&mut [u8]> or Option<&'static [u8]> (cannot move out of 'self.buf2'')
3: Since I don't really understand this comment, I am not sure if this is sound the way I implemented it. I think it is, but this comment irritates me.

@Sympatron
Copy link
Collaborator Author

Maybe it would be better to always return two slices instead of an Option. The second slice could just be empty if the buffer did not wrap.
That way implementing buf_mut() and as_static_buf for the second slice would be easier.

@Sympatron
Copy link
Collaborator Author

Sympatron commented Nov 5, 2020

I refactored it so that it does not need the Option anymore. This actually made the implementation and use of the SplitGrantR simpler.

Also added some basic sanity checks.

Copy link
Owner

@jamesmunns jamesmunns left a comment

Choose a reason for hiding this comment

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

Overall looks good, thanks for the PR! I made some comments, let me know what you think.

@Sympatron
Copy link
Collaborator Author

Implemented all the changed you supposed. Especially the borrow checker one was a good catch, Didn't think about that.

Copy link
Owner

@jamesmunns jamesmunns left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

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