tiborschneider / prefix-trie

Apache License 2.0
20 stars 4 forks source link

Add ascend() for get an iterator that generates supernet. #10

Closed kishiguro closed 2 months ago

kishiguro commented 3 months ago

This is useful to implement BGP aggregation.

tiborschneider commented 2 months ago

(Sorry for the late reply.) Thank you for this PR! I like the idea, but I have some comments on your implementation:

kishiguro commented 2 months ago

@tiborschneider thanks for prompt reply. just renamed ascend() to path() and other related structure. updated comment to explain when given prefix does not exists.

Let me ask a question about moving the implementation from ascend into Iterator. Since we don't have a parent index in the Node structure, we need to cache indices while traversing the tree. Should we do this every time next() is called, or should we do it when we instantiate the PathIterator?

grongor commented 2 months ago

Hello :wave:, I hope you don't mind if I weigh in :)

I don't fully agree with the name ascend, as that implies that the elements are yielded from the prefix to the root. Maybe path might be a better name?

If I understand the goal correctly, isn't that exactly what @kishiguro is trying to do? Given the example in the doc I'd say it is...I'd personally name the method supernets (and didn't include the searched prefix itself :smile:) to make it absolutely clear. Or maybe parents as there already is a method children (aka subnets).

And as I was writing this, I just now realized...isn't this functionality already provided by the cover method? I feel a bit lost now :smile:, so I may be completely off and just missing the point completely...

kishiguro commented 2 months ago

@tiborschneider oh, you are right, cover is the exactly what I wanted. sorry for taking your time. let me close this PR. thanks for letting me know the feature.

kishiguro commented 2 months ago

feature already implemented as cover