Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[runtime] Improve Split Functionality in DemiBuffer #546

Merged
merged 6 commits into from
Mar 21, 2023

Conversation

ppenna
Copy link
Contributor

@ppenna ppenna commented Mar 14, 2023

Description

This PR closes #542

Summary of Changes

  • Renamed DemiBuffer::split_off() to DemiBuffer::split_back()
  • Improved documentation for DemiBuffer::split_back()
  • Introduced DemiBuffer::split_front()
  • Introduced a test unit for DemiBuffer::split_front()

@ppenna ppenna added the enhancement Enhancement Request on an Existing Feature label Mar 14, 2023
@ppenna ppenna requested a review from iyzhang March 14, 2023 18:53
@ppenna ppenna self-assigned this Mar 14, 2023
@ppenna ppenna changed the title Enhancement runtime demibuffer [runtime] Improve Split Functionality in DemiBuffer Mar 14, 2023
@ppenna ppenna force-pushed the enhancement-runtime-demibuffer branch 2 times, most recently from cc83c47 to 2058821 Compare March 15, 2023 15:09
Copy link
Contributor

@iyzhang iyzhang left a comment

Choose a reason for hiding this comment

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

Looks good to me other than the duplicated code.

/// - The target [DemiBuffer] must be a single buffer segment (not a chain).
/// - The target [DemiBuffer] should be large enough to hold `offset`.
///
pub fn split_front(&mut self, offset: usize) -> Result<Self, Fail> {
Copy link
Contributor

Choose a reason for hiding this comment

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

To eliminate duplicate code, we should probably have a shared function here that gives both halves back and then split_front and split_back just return the front_half or back_half.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure... I merged the two functions in one, called split()

@ppenna ppenna force-pushed the enhancement-runtime-demibuffer branch from 2058821 to 66889f8 Compare March 17, 2023 23:36
This commit partially addresses
#542

- Renamed DemiBuffer::split_off() to DemiBuffer::split_back().
Improved API documentation for DemiBuffer::split_back().
This commit introduces minor enhancements in DemiBuffer::split_back():

- Introduced DemiBuffer::is_multi_segment() helper function.
- Simplified logic for sanity checking split_back() operation.
- Improved comments on the split_back() function.
Improved unit test for DemiBuffer::split_back().
This commit partially addresses
#542

- Introduced the DemiBuffer::split_front() operation.
- Introduced a unit test for DemiBuffer::split_front().
@ppenna ppenna force-pushed the enhancement-runtime-demibuffer branch from 66889f8 to 31edf07 Compare March 21, 2023 11:25
@ppenna ppenna merged commit f8ac135 into dev Mar 21, 2023
@ppenna ppenna deleted the enhancement-runtime-demibuffer branch March 21, 2023 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement Request on an Existing Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[runtime] Improve Split Functionality in DemiBuffer
2 participants