-
Notifications
You must be signed in to change notification settings - Fork 271
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
Rework container builder to use push into #569
Rework container builder to use push into #569
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, with some comments about potentially optional code, and some clarity / moments where I was confused as a reader.
Previously, `ContainerBuilder` had `push` and `push_container` functions, which were odd in the presence of the `PushInto` trait. This change removes the functions and instead relies on a `PushInto` implementation. As a bonus, this removes several `SizableContainer` bounds, which are now up to the caller to enforce should they push into a capacity-based container builder. Specifically, it adds the following new types or APIs: * ContainerBuilder: remove `push` and `push_container`. Replaces the fromer with a `PushInto` implementation, and moves the latter into a function on `CapacityContainerBuilder`. All uses of `give_container` are now special-cased to said builder. Other builders can accept containers through their `PushInto` implementation. * A `BufferingContainerBuilder` that only accepts complete buffers. Could back out that change because it's similar (but not equal!) to the capacity-based variant. * core::Input learns `new_input_with_buider` that allows the user to specify a customer builder. Existing APIs should function unchanged and use the capacity-based builder. * core::SharedStream uses the buffering container builder. * core::to_stream gains a `ToStreamBuilder` trait that allows the user to specify the builder. `ToStream` uses that but fixes the builder to the capacity-based variant. Signed-off-by: Moritz Hoffmann <[email protected]>
Signed-off-by: Moritz Hoffmann <[email protected]>
b1fde26
to
e639eb1
Compare
Signed-off-by: Moritz Hoffmann <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Merge at your discretion!
Signed-off-by: Moritz Hoffmann <[email protected]>
This fixes a potential bug in the consolidating container builder where it would not clear buffers that were exposed to the user. There is no obligation for the user to call clear, so call it when recycling. Signed-off-by: Moritz Hoffmann <[email protected]>
Previously,
ContainerBuilder
hadpush
andpush_container
functions,which were odd in the presence of the
PushInto
trait. This change removesthe functions and instead relies on a
PushInto
implementation. As abonus, this removes several
SizableContainer
bounds, which are nowup to the caller to enforce should they push into a capacity-based
container builder.
Specifically, it adds the following new types or APIs:
push
andpush_container
. Replaces the fromerwith a
PushInto
implementation, and moves the latter into a functionon
CapacityContainerBuilder
. All uses ofgive_container
are nowspecial-cased to said builder. Other builders can accept containers
through their
PushInto
implementation.BufferingContainerBuilder
that only accepts complete buffers. Couldback out that change because it's similar (but not equal!) to the
capacity-based variant.
new_input_with_buider
that allows the user tospecify a customer builder. Existing APIs should function unchanged and
use the capacity-based builder.
ToStreamBuilder
trait that allows the user tospecify the builder.
ToStream
uses that but fixes the builder to thecapacity-based variant.
Signed-off-by: Moritz Hoffmann [email protected]