refactor PathIterator::check_validity; add tests
#642
Conversation
| // If it's bigger than the remaining size, it's not in this range. | ||
| let mountain_size = (1 << (height + 1)) - 1; | ||
| if size >= mountain_size { | ||
| size -= mountain_size; |
There was a problem hiding this comment.
why not instead return immediately if mountain_size > size? [Edit: Oh does this mean there's no "peak" with the current height and hence no mountain of this size actually exists? If so please add a comment to the effect.]
Ah I see I didn't interpret "range" in your comment as "mountain range" but rather a more generic "range of values"! Perhaps reword your comment as:
// Subtract the size of the next mountain if it's part of this mountain range (mountain_size < size)
|
|
||
| // Height of the root of the smallest perfect binary | ||
| // tree containing `size` (leaf is height 0) | ||
| let mut height = 63 - size.leading_zeros(); |
There was a problem hiding this comment.
While this is more clear in isolation, given the rest of the iterator implementations maintain 2^(height+1) as "two_h" instead of converting height directly with each iteration, it might be best to stick with that & save a few cycles of computation each iteration.
|
General nit: Please format comments to 100 columns to match rest of the file. (If you use vscode I recommend the Rewrap extension.) |
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #642 +/- ##
========================================
Coverage 89.41% 89.42%
========================================
Files 144 145 +1
Lines 36754 37033 +279
========================================
+ Hits 32865 33117 +252
- Misses 3889 3916 +27
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
I have an easier time conceptualizing the validity condition in the way I described in the comments. If you feel like this will make the code easier to understand, great. Otherwise, feel free to just keep the tests.