refactor: use more gsl::not_null in utils.h and deterministicmns.h #5651
Conversation
PastaPastaPasta
force-pushed
the
more-not-null
branch
from
2e086cd
to
a3bdc56
Compare
| if constexpr (std::is_same<T, CBLSPublicKey>()) { | ||
| assert(false); | ||
| } | ||
| static_assert(!std::is_same<T, CBLSPublicKey>(), "GetUniquePropertyHash cannot be templated against CBLSPublicKey"); |
Please, review #5590 first |
|
This pull request has conflicts, please rebase. |
| @@ -46,38 +47,38 @@ namespace utils | |||
| { | |||
|
|
|||
| // includes members which failed DKG | |||
| std::vector<CDeterministicMNCPtr> GetAllQuorumMembers(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, bool reset_cache = false); | |||
| std::vector<CDeterministicMNCPtr> GetAllQuorumMembers(Consensus::LLMQType llmqType, gsl::not_null<const CBlockIndex*> pQuorumBaseBlockIndex, bool reset_cache = false); | |||
There was a problem hiding this comment.
so far as this and many other places requires not-null anyway, why even don't change const CBlockIndex*> to the const CBlockIndex&` ?
For reference, couple PR with bitcoin's similar changes of CBlockIndex* to CBlockIndex&. Bitcoin doesn't change it everywhere, but only when it is obviously non-nullptr, such as:
https://github.com/bitcoin/bitcoin/pull/25677/files
https://github.com/bitcoin/bitcoin/pull/25016/files
There was a problem hiding this comment.
Well; so one benefit imo is that dereferencing a nullptr is undefined behavior not guaranteed to be caught at runtime; additionally, gsl::not_null doesn't require additional syntax to pass to a later func that needs a ptr or additional syntax when passing in a ptr. In bitcoin PR you mention it appears they force to use something like *Assert(m_chain.Tip() which is significantly more typing :D
There was a problem hiding this comment.
well, *Assert(...) is 7 bytes shorter than gsl::not_null<...>(...)
And indeed, you can avoid UB by validating it before converting to reference.
There was a problem hiding this comment.
Yeah; but *Assert you have to have at every call site instead of just function definition. They're both generally valid approaches imo
There was a problem hiding this comment.
yeah, but once you convert pointer to reference, you can assume that all recursive calls; submethods; helpers, etc are never have nullptr anymore; but for case of pointer - you need to add gsl:not_null to every method do show that is not nullptr.
There was a problem hiding this comment.
Well yes; same as every method needs to be declared as a reference. Once a not_null is constructed it is zero cost to pass it to another not_null function
PastaPastaPasta
force-pushed
the
more-not-null
branch
from
a3bdc56
to
3d75580
Compare
Issue being fixed or feature implemented
Use not_null if the function would crash if given a nullptr
What was done?
Refactored to use gsl::not_null
How Has This Been Tested?
Compiled
Breaking Changes
Should be none
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.