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

[PR 1/5] Organize Buffer's segments as a regular list #332

Merged
merged 3 commits into from
Jun 21, 2024

Conversation

fzhinkin
Copy link
Collaborator

Previously, Buffer's segments were organized into a circular list.
That allowed storing only a single reference to buffer's head, and also facilitated insertion/removal of new list nodes.
The downside of a circular list is that one has to always compare a current node with a head when iterating over segments.

That complicates the implementation of a public API for segments iterations (See #135 (comment) for details on segment iteration API).

This PR changes how segments are organized within a buffer to no longer form a circular list. Head's previous node and tail's next node now will always be null.

Previously, Buffer's segments were organized into a circular list.
That allowed storing only a single reference to buffer's head,
and also facilitated insertion/removal of new list nodes.
The downside of a circular list is that one has
to always compare a current node with a head when
iterating over segments.
That complicates the implementation of a public API
for segments iterations.

See #135 (comment)
for details on segment iteration API.
@fzhinkin fzhinkin changed the title Organize Buffer's segments as a regular list [PR 1/5] Organize Buffer's segments as a regular list Jun 6, 2024
} else {
out.head!!.prev!!.push(copy)
out.tail = out.tail!!.push(copy)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should create some private inline methods for consistent mutating of a buffers?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Extracted this copy-related logic to a separate function. Thanks for the suggestion

core/common/src/ByteStrings.kt Show resolved Hide resolved
@lppedd
Copy link
Contributor

lppedd commented Jun 12, 2024

The only thing I don't like about this code is the amount of !! assertions, especially in loops.
I see they were already present before, and I haven't checked what happens in subsequent PRs, but maybe another pattern, instead of having to deal with nulls, could be useful.

@fzhinkin fzhinkin merged commit 6f95a1b into develop Jun 21, 2024
1 check passed
@fzhinkin fzhinkin deleted the organize-segments-as-list branch August 27, 2024 16:36
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.

3 participants