swarm/pot: pot.remove fixed #1098
Conversation
gluk256
commented
Jan 9, 2019
gluk256
commented
- bugfix for pot.remove() function
- new tests for pot.remove()
- fixed the comments
swarm/pot/pot.go
Outdated
| // Remove deletes element v from the Pot t and returns three parameters: | ||
| // 1. new Pot that contains all the elements of t minus the element v; | ||
| // 2. proximity order of the removed element v; | ||
| // 3. boolean indicating if the item was found. |
| @@ -201,7 +195,7 @@ func remove(t *Pot, val Val, pof Pof) (r *Pot, po int, found bool) { | |||
| } | |||
| bins = append(bins, t.bins[j:]...) | |||
| r = &Pot{ | |||
| pin: val, | |||
| pin: t.pin, | |||
| @@ -201,7 +195,7 @@ func remove(t *Pot, val Val, pof Pof) (r *Pot, po int, found bool) { | |||
| } | |||
| bins = append(bins, t.bins[j:]...) | |||
| r = &Pot{ | |||
There was a problem hiding this comment.
why not return the same pot if not found?
There was a problem hiding this comment.
because the function is recursive, so if not found at the current level of pot tree, it might be be found deeper.
There was a problem hiding this comment.
But if not found the contents of the newly created Pot object is identical to the one passed, no? Is the intention that the Pot object that is passed to this (and similar) functions is discarded (only use the returned Pot)?
| t.Fatalf("incorrect indexes in iteration over Pot. Expected %v, got %v", exp, got) | ||
| } | ||
| got = fmt.Sprintf("%v", po) | ||
| exp = "[3 2 1 0 0]" |
There was a problem hiding this comment.
I don't understand why these po's are correct.
If the pin has its highest-order bit set to 1, shouldn't any address with highest-order bit 0 mean po == 0?
Following the logic of this expectation, shouldn't it be rather [5 3 2 0]?
Why are there 5 elements after deleting one? Does that mean we count the pin aswell? If so, what does the po actually mean here? Is it po in relation to the pin? And if it is, why isn't there a po == 256 first, which would be the pin address compared to itself?
There was a problem hiding this comment.
po is calculated in relation to the parent node in the pot
There was a problem hiding this comment.
Your answer doesn't clear up my questions, I'm afraid.
What is "parent node." Is it the same as the pin?
| } | ||
| } | ||
|
|
||
| func TestPotRemoveSameBin(t *testing.T) { |
There was a problem hiding this comment.
Please explain what the tests are doing in a comment
| } | ||
| inds, po = indexes(n) | ||
| got := fmt.Sprintf("%v", inds) | ||
| exp := "[1 2 3 5 0]" |
There was a problem hiding this comment.
this is how this test function indexes() works. otherwise it's irrelevant.
There was a problem hiding this comment.
Can you elaborate, please? Are you saying that the order doesn't matter? Why doesn't the order matter?
| t.Fatalf("incorrect indexes in iteration over Pot. Expected %v, got %v", exp, got) | ||
| } | ||
| got = fmt.Sprintf("%v", po) | ||
| exp = "[0 1 2 4 0]" |
There was a problem hiding this comment.
What does the last 0 represent here?
There was a problem hiding this comment.
So we have "root," "parent node," and "pin." Are they the same thing? Please let's use one term.
Grep'ing the files doesn't really clear up the terms either:
$ grep -i "parent" swarm/pot/ -r
swarm/pot/pot_test.go:// child_po = parent_po + 1.
$ grep -i "root" swarm/pot/ -r
swarm/pot/pot_test.go:// log.Root().SetHandler(log.LvlFilterHandler(log.LvlTrace, log.StreamHandler(os.Stderr, log.TerminalFormat(false))))
swarm/pot/pot_test.go:// this test creates a flat pot tree (all the elements are leafs of one root),
swarm/pot/pot.go:// Pot is the node type (same for root, branching node and leaf)
swarm/pot/pot.go:// the function argument is passed the value and the proximity order wrt the root pin
swarm/pot/pot.go:// it does NOT include the pinned item of the root
"root pin" .. "pinned item of root" ...
There was a problem hiding this comment.
Cleared up pending comments on call, and attempting a summary of additional questions that arise from this PR, which are out of scope for it (which is why I approve it):
The unexpected po results in the tests arise from the fact that in the case of a hierarchical Pot (TestRemoveSameBin) the po values reported are distances between the added elements, whereas the flat Pot (TestRemoveDifferentBins) the po values are the distances between the added elements and the pinned value. It's not currently clear whether this is a feature or bug.
Also, the po value of the pinned value is 0 because it's explicitly set to this. This value has no meaning for the pinned address, since the Pot is hierarchical, and the pinned value, representing the root of the hierarchy, has nothing to compare its po to. However, this po is also included in the result arrays in the test, which is nonsensical at best (po for pinned value has no meaning) to inconsequential at worst (po is sometimes in relation to pinned value, sometimes not, and in the former case the value should be 256 instead).
Furthermore, there is a confusion of terms involved with the "root" of the Pot - variations include:
- pin
- root
- root pin
- pinned item from root
- parent node
it's unclear whether or not all these terms refer to the same thing. The nomenclature needs to be clear and unambiguous.
Lastly: The functions in pot really should be commented properly (e.g. add), and understanding what they do is very difficult.
nolash
added
ready to submit to ethereum/go-ethereum
stability
and removed
ready for review
labels
|
merged as ethereum/go-ethereum#18431 |
