fix: comment for NextHighestPowerOf2 #644
Conversation
Codecov Report
@@ Coverage Diff @@
## main #644 +/- ##
==========================================
+ Coverage 29.78% 38.45% +8.67%
==========================================
Files 16 19 +3
Lines 2055 2538 +483
==========================================
+ Hits 612 976 +364
- Misses 1382 1476 +94
- Partials 61 86 +25
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
x/payment/types/payfordata.go
Outdated
| @@ -188,8 +188,7 @@ func powerOf2MountainRange(l, k uint64) []uint64 { | |||
| return output | |||
| } | |||
|
|
|||
| // NextHighestPowerOf2 returns the next lowest power of 2 unless the input is a power | |||
| // of two, in which case it returns the input | |||
| // NextHighestPowerOf2 returns the next highest power of 2 | |||
There was a problem hiding this comment.
Not sure this is the correct resolution. If anything, the function should be renamed to NextLowestPowerOf2? Additionally, the linked issue should be resolved by properly documenting behavior when the input is a power of 2.
These are two orthogonal things.
There was a problem hiding this comment.
- This function finds the next highest power of two. Confirmed by checking these tests
- I think it is more confusing to explicitly document the behavior when the input is a power of 2 because the behavior is the same as when the input is not a power of 2. In both cases, this function returns the next highest power of 2. I think examples may be the easiest way to convey the behavior
// NextHighestPowerOf2 returns the next highest power of 2. Examples:
// NextHighestPowerOf2(1) = 2
// NextHighestPowerOf2(2) = 4
// NextHighestPowerOf2(11) = 16
func NextHighestPowerOf2(v uint64) uint64 {thoughts?
There was a problem hiding this comment.
UPDATE: now I see why highest is misleading here 🤦♂️ this function returns the next lowest power of 2 that is higher than the input. Hmm will re-think comments / function naming here
There was a problem hiding this comment.
NextHigherPowerOf2 would also work.
There was a problem hiding this comment.
Will avoid renaming because there exists a nextLowestPowerOf2 already:
celestia-app/x/payment/types/payfordata.go
Line 218 in 127f990
|
Will merge this but happy to create a follow-up PR if @adlerjohn has any other feedback |
|
It's fine as-is, but could you file a follow-up issue @rootulp so we don't forget? |
|
Sure #648 |
Closes informalsystems/audit-celestia#4