The unpatched if condition is not robust to additions of new states of AddProofStatus.
This patch makes the condition more explicit which is easier to read and more robust to
future changes.
Details
Details
- Reviewers
deadalnix - Group Reviewers
Restricted Project
ninja check-avalanche
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- explicit-proof-replacement-check
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 18979 Build 37720: Build Diff build-diff · lint-circular-dependencies · build-debug · build-without-wallet · build-clang · build-clang-tidy Build 37719: arc lint + arc unit
Event Timeline
| src/avalanche/peermanager.cpp | ||
|---|---|---|
| 262 ↗ | (On Diff #33428) | What if it is duplicated? |
| src/avalanche/peermanager.cpp | ||
|---|---|---|
| 262 ↗ | (On Diff #33428) | That should not be possible since we are already in the REJECTED case of addProofIfNoConflict. If it was duplicated, we'd find ourselves in the DUPLICATED case statement (see around line 305). |
| src/avalanche/peermanager.cpp | ||
|---|---|---|
| 262 ↗ | (On Diff #33428) | That assumes the implementation of the function. What if it was duplicated, would you want to run this code or not? |
Comment Actions
As per our discussion, I think the main problem with this is that, while the existing code indeed looks funny, not in a good way, it's not clear what the correct structure actually is. I think we should put this on hold untill we actually have a a problem that would benefit from restructuring this code, then we'll have a better idea of what it should look like.