Relative parsing navigation in fdt_rs::base

[Kritzefitz]

Currently fdt_rs::base only allows you to extract nodes themselves and their properties, but not to interrogate the tree structure between those node.

This creates a slightly awkward situation when searching for memory nodes to set up dynamic allocations. I would like to use structural information for this, because at this stage I'm only interested in memory nodes directly below the root. Not any that might be lingering around deeper into the structure. I wouldn't want to use fdt_rs::index, because I can't make dynamic allocations yet and creating some kind of static allocation would require me to make more assumptions about the memory layout before I can actually parse it. This can all be worked around, but I think we can do better.

This branch beefs up DevTreeIter a little, so we can instruct it to only return nodes and props in certain scopes relative to the current position. This is used by DevTreeNode to implement functions that can return iterators over

• a node's recursive descendants,
• a node's direct children,
• a node's recursive descendants, it's later siblings and all their recursive descendants and
• a node's later siblings.

It's slightly awkward that we can only return siblings from the current node onward and not previous siblings. But I don't see a way to do this efficiently without allocations, as we'd have to be able to remember and jump back to an arbitrary number of points along the parsed stream.

There's another slightly awkward limitation, that we have siblings_and_descendants which includes the current node's descendants, but no variant of that that excludes the current nodes descendants and only returns those of the siblings. This doesn't seem straightforward to implement without introducing another state parsing flag for just this use case. Since I don't have a practical use case for this at the moment, I didn't bother too much to implement it. If desired, the missing behavior can be emulated by calling next_sibling on a node and then calling siblings_and_descendants on that.

[spwilson2]

Thanks for the PR - I appreciate all the included documentation! I took a quick glance and my initial thought is that I’ll probably request we create a separate ‘heavier’ iterator to handle this logic and leave the current one as super simple/lightweight one.

But I’ll take time to actually review this weekend.

[spwilson2]

Okay I took an in-depth look.

I'm definitely really happy with the intent behind the extension. Thanks a bunch for contributing it! =)

I agree that all the filters seem incredibly useful and the current implementation lacks necessary filters. However, I don't love the idea of modifying the base iterator to include the filters baked it. To me it seems bloat the responsibility of what ideally would be a relatively simple iterator. On the external side, it may be a little confusing to users as the type doesn't indicate what limits are associated with its iteration pattern.

Additionally, since nodes rely on a copy of this iterator in order to implement their own methods. Unexpected issues could arise. E.g. I believe the DevTreeNode.find_next_compatible_node method now behaves different depending on which iterator filter was used to iterate to the given node.

I believe we could add similar functionality by creating a wrapper iterator (or collection of them) which includes all the added fields and simply uses the current implementation of DevTreeIter under the hood. If a depth value would be helpful to add to DevTreeIter, I wouldn't be opposed to that. I'd just like to avoid adding filters to this base iterator type.

[Kritzefitz]

You make a very good point. I'll try to move the complex filtering logic into separate iterators.

[Kritzefitz]

I've been thinking about how to implement this with additional iterator types. But when trying out concrete ideas, it always seems to me like I need to either break API, make the APIs of the new types inconsistent to the current APIs or write a bunch of boilerplate code.

The thing is, DevTreeIter currently implements next_item, next_prop, next_node, next_node_prop, and next_compatible_node. If we implement additional scope-specific iterators, say DevTreeChildrenIter, DevTreeDescendanstIter DevTreeSiblingsAndDescendantsIter, and DevTreeSiblingsIter, they would basically do the same things, so they should probably support the same set of operations. Also we would need those operations to support pendants of DevTree{Node,Prop,NodeProp}Iter on those new types.

Thinking about the type-specific iterators in the context of introducing new scope-specific iterators opens up a combinatorial explosion. Because now we also need DevTreeChildrenNodeIter, DevTreeChildrenPropIterator, DevTreeDescendantsNodeIter and all the other combinations... The obvious solution would be to parametrize the type-specific iterators over the iterator they're filtering from. So the current DevTreeNodeIter would become DevTreeNodeIter<DevTreeIter> and we could trivially make DevTreeNode<DevTreeChildrenIter> from that, as long as the underlying iterators have a common trait to make that possible.

However, I don't think that paremetrization is possible without breaking API or introducing weird type aliases for backwards-compatibility. Note that struct DevTreeNodeIter<I = DevTreeIter<'a, 'dt>> probably wouldn't work, because the lifetime parameters wouldn't be in scope.

While not a combinatorial explosion, implementing the four scope-specific iterators also seems like it would generate a lot of boilerplate, since we'd have to write an implementation of next_item, next_prop, next_node, next_node_prop and next_compatible_node for each of them, that would essentially do the same. I think a lot of this could be avoided, by making a new trait that I will call DevTreeIterTrait until I come up with a better name. To implement it, you would only need to provide next_item and would get the rest of the operations as provided methods. However, this would also require breaking API, because all those methods would then move from DevTreeIter to DevTreeIterTrait and only be in scope if the trait is in scope.

I guess an API compatible solution might exist, by moving the logic currently in DevTreeIter to a new private type DevTreeIterBase which would implement FallibleIterator with Item = (DevTreeItem, isize). We could then use standard methods of FallibleIterator to do whatever filtering we like. Then we could have a DevTreeIterAPIWrapper<I> where I could be any FallibleIterator with Item = (DevTreeItem, isize), which would then provide all the next_* methods and an implementation for FallibleIterator with Item = DevTreeItem. DevTreeIter and the scope-scpecific variants would then become simple wrappers around DevTreeIterAPIWrapper<I> with some I to hide the complex typing involved. However, when I tried to prototype this, it involved more refactoring than I'm comfortable to do right now. It also seems like the resulting API would be a lot more confusing than the approach using a trait above.

Would you be willing to accept API breaking changes for this? If not, which of the options seems best to you, to keep API compatibility? Or maybe you have some different ideas altogether?

[Kritzefitz]

Ok, I just pushed new version of my changes using new iterator types as you suggested. Feel free to ignore my previous comment about the change being complicated. I think in some regards I was overthinking things.

Some points to note about the new design:

• DevTree{Node,Prop,NodeProp}Filter functionally overlap with DevTree{Node,Prop,NodeProp}Iter, but I kept the latter to stay API compatible.
• siblings_and_descendants now returns only the siblings and their descendants and not the current descendants. This is both more intuitive than the previous behavior and was easier to implement using the current approach.

[coveralls]

Pull Request Test Coverage Report for Build 3307211367

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 109 of 177 (61.58%) changed or added relevant lines in 2 files are covered.
• 2 unchanged lines in 1 file lost coverage.
• Overall coverage decreased (-0.05%) to 59.672%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/base/node.rs	15	27	55.56%
src/base/iters.rs	94	150	62.67%

Files with Coverage Reduction	New Missed Lines	%
src/base/iters.rs	2	70.34%

Totals
Change from base Build 3200203696:	-0.05%
Covered Lines:	654
Relevant Lines:	1096

---

💛 - Coveralls