--- Log opened Sun May 10 00:00:16 2020
05:10 < adiabat> ja: rise > forestRows would be  invalid, yes, but it doesn't make sense to have this condition
05:10 < adiabat> that means you want to go up n rows, but there are less than n rows total in the forest
05:11 < adiabat> it could return an error?  But there are lots of things in utils that could return an error... for parentMany, it would be
05:12 < adiabat> if position > (1<<forestRows)
05:12 < adiabat> if rise > forestRows
05:13 < adiabat> if rise == 0 (well might still work but do nothing?)
05:14 < adiabat> similarly in childMany you can have drop > forestRows and that would do something nonsensical
05:15 < adiabat> hm I guess in childMany if drop > forestRows it just returns 0, which at least seems ... better?  Still wrong though
05:17 < adiabat> for too large rise values, parentMany would also return 0 :
05:17 < adiabat> (position>>rise) would be 0 since rise is bigger the row of any (valid) position
05:19 < adiabat> the (mask << forestRows-(rise-1)) part could get either huge or 0 depending on how big the underflow shift is
05:19 < adiabat> but then the whole thing is & mask at the end, which makes everything 0
05:19 < adiabat> so it will give the wrong answer, but there is no right answer
05:20 < adiabat> I get the desire for more assertions, like, position < 1<<forestRows, or rise < forestRows, but I don't think putting errors here is the best way to do it...
05:22 < adiabat> just because then you have all these calls to child, parent, detectOffset, inForest, etc, and they can all be given invalid arguments
05:24 < adiabat> so if we want assertions about valid inputs, have them upstream?  Or panic.  I could be OK with panic, since something is seriously broken if parentMany is getting called that way...
12:42 < ja> yeah, it is fine to panic on broken assumptions
12:43 < ja> i will submit a PR one of these days. but it will break the tests...
12:44 < ja> returned errors only make sense when it is e.g. a user error. but this is internal, the caller should not violate the assumptions
14:06 < ja> adiabat: i am not sure how to fix getRootsReverse which breaks this assumption... i tried something but now it loops infinitely
15:01 < ja> adiabat: you say that &mask would make everything zero, but my tests do not confirm that. i get e.g. 6, 14 and 30 returned if the rist>forestRows precondition is violated ( which it is)
15:29 < adiabat> ja: getRootsReverse could break if leaves > (1<<forestRows).  It will ignore the leaves in excess of 1<<forestRows 
15:31 < adiabat> huh I guess I don't see cases where it parentMany returns non-zero but this is just looking at the code, which is a bit hard to follow
15:31 < adiabat> I guess there's a way to get (mask << uint64(forestRows-(rise-1)))) & mask to be non-zero, huh
15:32 < adiabat> anyway in all these functions, position > (1<<forestRows) is not OK
15:32 < adiabat> and rise or drop > forestRows also not OK
15:33 < adiabat> forestRows is actually an *almost* redundant variable
15:33 < adiabat> in 99.9% of the time, you could just call treeRows() on number of leaves, and that will give you forestRows
15:34 < adiabat> well then the arguments would just be position and numLeaves, which isn't any simpler I guess
16:30 < ja> adiabat: do you get a panic if you insert "if row+1 > forestRows { panic(...) }" in parentMany and run the test suite?
21:25 -!- achow101 [~achow101@unaffiliated/achow101] has quit [Remote host closed the connection]
21:26 -!- achow101 [~achow101@unaffiliated/achow101] has joined #utreexo
--- Log closed Mon May 11 00:00:17 2020