Conversation
This changes the `Node` and `Property` traits so that instead of requiring external lifetimes, the lifetimes are defined by the trait implementations themselves. This is mainly useful for unifying the `Fdt` APIs (which borrow the data) and `DeviceTree` APIs (which own the data). Furthermore, this will be useful when we introduce mutable `Fdt` in-place APIs, since `&mut self` methods cannot return non-self-borrowed data. With the old version this would require implementing the mutating traits for `&T` rather than for `T` as we did with `DeviceTree` trait impls. The main downsides is increased complexity of the traits.
| /// An iterator over the children of a device tree node. | ||
| enum FdtChildIter<'a> { | ||
| #[derive(Debug, Clone)] | ||
| pub struct FdtChildIter<'a>(FdtChildIterInner<'a>); |
There was a problem hiding this comment.
Why do you need this extra wrapper type around the iterator?
There was a problem hiding this comment.
The reason is that because now it appears as a type in a trait implementation, it needs to be public. I didn't want to expose the internals of the implementation though (i.e. iterator enum variants) so that we don't overpromise in the public API.
However, a better idea is probably to make it unnameable type instead by putting it in a private module, so that's what I'll do instead.
|
|
||
| impl<'a> Node<'a> for &'a DeviceTreeNode { | ||
| type Property = &'a DeviceTreeProperty; | ||
| impl Node for DeviceTreeNode { |
There was a problem hiding this comment.
Why does the trait need to be implemented both for DeviceTreeNode and &DeviceTreeNode?
There was a problem hiding this comment.
The owned version is mostly provided for convenience, so that you don't have to wrap an owned node in a reference to call methods on it (node.child("foo") instead of (&node).child("foo").
| fields_cells: [usize; N], | ||
| ) -> Result<crate::values::PropEncodedArrayIterator<'a, N>, PropertyError> { | ||
| crate::values::PropEncodedArrayIterator::new(&self.value, fields_cells) | ||
| } |
There was a problem hiding this comment.
There's more code duplication here too, can we avoid it somehow?
There was a problem hiding this comment.
We could move this to a macro at least.
| Ok(Memory { node }) | ||
| } | ||
|
|
||
| /// Returns the `/reserved-memory/*` nodes, if any. |
There was a problem hiding this comment.
This is no longer true, it now returns the node itself rather than its children.
|
|
||
| /// Typed wrapper for a `/reserved-memory` node. | ||
| #[derive(Clone, Copy, Debug)] | ||
| pub struct ReservedMemoryNode<N> { |
There was a problem hiding this comment.
Why is this necessary? As far as I understand the /reserved-memory node won't have any properties of its own, only its children are relevant, so why do we need a type for the node itself?
There was a problem hiding this comment.
When writing the initial version of the code, I needed it because of some lifetime issues. Now it indeed is not needed (I guess I made some lifetime-related changes in the meantime that allowed this), so I just restored the previous version of the code - thanks for catching this!
|
Please also fix the clippy failure. |
2 participants
This changes the
NodeandPropertytraits so that instead of requiring external lifetimes, the lifetimes are defined by the trait implementations themselves. This is mainly useful for unifying theFdtAPIs (which borrow the data) andDeviceTreeAPIs (which own the data).Furthermore, this will be useful when we introduce mutable
Fdtin-place APIs, since&mut selfmethods cannot return non-self-borrowed data. With the old version this would require implementing the mutating traits for&Trather than forTas we did withDeviceTreetrait impls.To make things easier for users, specific types are used everywhere in the implementations of the traits, so that
rustdocclearly displays the actual return types.The main downsides is increased complexity of the traits and increased code duplication in some places.