Skip to content

Conversation

practicalswift
Copy link
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.

@maflcko maflcko closed this Aug 17, 2018
@maflcko maflcko reopened this Aug 17, 2018
@promag
Copy link
Contributor

promag commented Aug 17, 2018

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
Copy link
Contributor Author

practicalswift commented Aug 17, 2018

@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 :-)

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 17, 2018

No more conflicts as of last run.

@practicalswift practicalswift deleted the document-non-nullptr-assumptions branch April 10, 2021 19:35
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants