turbine::broadcast_stage: then_some/ok_or_else => if
#1301
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1301 +/- ##
=========================================
- Coverage 81.6% 81.5% -0.1%
=========================================
Files 867 867
Lines 368900 368901 +1
=========================================
- Hits 301052 300999 -53
- Misses 67848 67902 +54 |
| self.slot = bank.slot(); | ||
| self.parent = bank.parent_slot(); | ||
| if self.slot != bank.parent_slot() { |
There was a problem hiding this comment.
This does not look right. This is effectively checking:
if bank.slot() != bank.parent_slot() {which is not equivalent to the old code.
There was a problem hiding this comment.
Fixed.
Thank you for double-checking.
I moved the assignment into the "update state" block and missed the point that self.slot was updated %)
ilya-bobyr
force-pushed
the
pr/turbine-StandardBroadcastRun-use-if
branch
2 times, most recently
from
e8630cd to
de24b90
Compare
`if/else` seems easier to follow, as it produces less nesting. Assignment of `self.chained_merkle_root` into `chained_merkle_root`, that is then assigned back into `self.chained_merkle_root` is a no-op. While I can see how it is separating the computation of the `chained_merkle_root` for the current step from the update of the `self`, I think, overall, it is more confusing then helpful. The `Finish previous slot ...` comment is actually part of a multi-paragraph sequence of steps, that document the whole `process_receive_results()`. Step number was accidentally removed in `anza-xyz#1057`: commit 6b45dc9 Author: behzad nouri <[email protected]> Date: Fri May 3 20:20:10 2024 +0000 retains chained Merkle root across leader slots (anza-xyz#1057)
ilya-bobyr
force-pushed
the
pr/turbine-StandardBroadcastRun-use-if
branch
from
de24b90 to
9e25cbd
Compare
| ) | ||
| }) | ||
|
|
||
| if self.slot != bank.parent_slot() { |
There was a problem hiding this comment.
As mentioned on slack, I appreciate you looking into this code, and it is always good to have more eyes on the code and try to spot bugs or inefficiencies.
However this is only a "style" change and people's personal preference on what style is better can be different.
I personally prefer the old code because it avoids this if branch which changes state, whereas the old code uses an "expression". This type of state changing if branches tend to be a source of bugs and errors and I personally rather avoid them when possible.
Problem
A few spots could be clarified.
Summary of Changes
if/elseseems easier to follow, as it produces less nesting.Assignment of
self.chained_merkle_rootintochained_merkle_root, that is then assigned back intoself.chained_merkle_rootis a no-op. While I can see how it is separating the computation of thechained_merkle_rootfor the current step from the update of theself, I think, overall, it is more confusing than helpful.The
Finish previous slot ...comment is actually part of a multi-paragraph sequence of steps, that document the wholeprocess_receive_results(). Step number was accidentally removed in #1057.