Remove BatchContainer::{copy, copy_range}.
#508
Conversation
| @@ -510,7 +500,7 @@ pub mod containers { | |||
| use crate::trace::IntoOwned; | |||
|
|
|||
| /// A general-purpose container resembling `Vec<T>`. | |||
| pub trait BatchContainer: 'static { | |||
| pub trait BatchContainer: for<'a> PushInto<Self::ReadItem<'a>> + 'static { | |||
There was a problem hiding this comment.
Discussed in person: We could opt out of requiring BatchContainer to be PushInto<...> which would permit implementing containers that cannot be pushed at. For the moment, the change as implemented is probably the right thing to do, but in the future we could split the trait into ReadBatchContainer and BatchContainer where the former does not expose any functionality to modify the contents of the container. There's a similar idea in flatcontainer: antiguru/flatcontainer#27
There was a problem hiding this comment.
Yup. I think this is a non-regression in the sense that copy prevents a read-only container also, and the main change is that in this PR that logic must be done through PushInto, and .. idk perhaps it's hard to use it for the implementation. But .. as long as it works for our current containers, great and it seems easy to roll back at any point.
|
Thanks for the review! |
This PR replaces
BatchContainer::copywith aPushIntorequirement. We could leave thecopymethod while insisting on thePushIntoimplementation, if that provides more clarity about when we are specifically copying back aReadItem, but it felt best to use the one idiom we have for implementing pushing into a container, vs having multiple implementations around (which we had).BatchContainer::copy_rangewas removed because no one is using it. No harm adding it back, but in that case perhaps we instead want to supportPushIntofor a range type (or some wrapper around one).