Document assumptions made in PeerLogicValidation::SendMessages(...) and rescanblockchain(...) #13548

pull practicalswift wants to merge 2 commits into bitcoin:master from practicalswift:document-non-nullptr-assumptions changing 2 files +3 −0

practicalswift commented at 4:46 PM on June 27, 2018: contributor

Document assumptions made in PeerLogicValidation::SendMessages(...) and rescanblockchain(...) via assertions.

The fact that pBestIndex != nullptr and stopBlock != nullptr in these contexts is not immediately obvious. Explicit is better than implicit.
Document assumption made about pBestIndex in PeerLogicValidation::SendMessages(...) e30ef3e521
Document assumption made about stopBlock in rescanblockchain(...) a7946bc261
fanquake added the label Refactoring on Jun 27, 2018
MarcoFalke closed this on Aug 17, 2018
MarcoFalke reopened this on Aug 17, 2018
promag commented at 4:00 PM on August 17, 2018: member

is not immediately obvious. Explicit is better than implicit.

I don't agree, it's obvious because they are used right after the assertions.
practicalswift commented at 4:36 PM on August 17, 2018: contributor

@promag Yes, but the assertion documents that the dereference without a prior null check is intentional and not just an oversight. That helps static analyzers and human reviewers, don’t you agree? :-)

FWIW I’ve seen numerous static analyzers independently flag these specific cases (and the cases covered by the other similar assertion PR:s). I’m not just adding asserts randomly for fun :-)
practicalswift cross-referenced this on Aug 31, 2018 from issue wallet: Remove unused local variable old_label by practicalswift
DrahtBot cross-referenced this on Sep 17, 2018 from issue Remove unnamed block in SendMessages function by kostyantyn
DrahtBot commented at 4:15 PM on September 17, 2018: contributor

No more conflicts as of last run.
practicalswift closed this on Oct 18, 2018
ajtowns cross-referenced this on Oct 15, 2020 from issue Make Assert(…) usable in all contexts. Make implicit assumptions explicit. by practicalswift
practicalswift deleted the branch on Apr 10, 2021
bitcoin locked this on Aug 16, 2022

Contributors

Labels

Linked (view graph)

#14117 wallet: Remove unused local variable old_label #20122 Make Assert(…) usable in all contexts. Make implicit assumptions explicit.