Conversation
m4tx
force-pushed
the
model
branch
2 times, most recently
from
9e8fc09 to
57db4ed
Compare
m4tx
force-pushed
the
model
branch
2 times, most recently
from
e283557 to
bb4e565
Compare
| pub mod error; | ||
| pub mod fdt; | ||
| pub mod memreserve; | ||
| #[cfg(feature = "write")] |
There was a problem hiding this comment.
It looks enabling the alloc feature without write has no meaningful effect, why not just have a single feature?
There was a problem hiding this comment.
I thought we might eventually have some functionality that will require the alloc crate, but won't be used for the intermediate DT representation (which requires its own set of additional libraries). For instance, in-place FDT modification might optionally require alloc crate to support extending the FDT size to relocate nodes when adding new data - but this is indeed too forward-looking and we can just re-introduce the feature flag then.
| /// ``` | ||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub struct DeviceTree { | ||
| pub(self) root: DeviceTreeNode, |
There was a problem hiding this comment.
Isn't pub(self) the same as omitting pub entirely?
There was a problem hiding this comment.
Hmm, for some reason I thought that this makes the property visible in the child modules (which isn't even utilized in this PR, but at some point was in the next one), but indeed I was wrong. I'll just make this pub and remove the getter/setter methods (and add #[non_exhaustive] to the struct to avoid API breakages when we add new stuff here).
| /// | ||
| /// ``` | ||
| /// # use dtoolkit::model::{DeviceTree, DeviceTreeNode}; | ||
| /// let root = DeviceTreeNode::new("/"); |
There was a problem hiding this comment.
What happens if someone passes a node with a different name as the root? If there's no reason to do that, why not just construct the appropriate DeviceTreeNode within this new method?
There was a problem hiding this comment.
Right, the device tree spec says:
The devicetree has a single root node of which all other device nodes are descendants. The full path to the root node is /.
I'll change this to create an appropriate root node here then. This isn't ideal, because the user can still do tree.root = DeviceTreeNode::new("foobar"), but I guess that's fine.
2 participants
No description provided.