-
Notifications
You must be signed in to change notification settings - Fork 35.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add not_null<T>
from the Guidelines Support Library
#24423
Comments
@theuni @ryanofsky @ajtowns you might have opinions / thoughts? |
I guess I'd be curious to see real examples of where it is useful. It seems to me like most of the places where you could use I think in general replacing const CChain& active = Assert(m_node.chainman)->ActiveChain(); with const CChain& active = m_node.chainman->ActiveChain(); You get same runtime assert in either case here, but in first case author is clearly stating intention for chainman to be not null here at this line of the code, while in second case with |
To turn runtime errors into compile errors #19273 ? |
Is #19273 the right issue? I think point of not_null has more to do with safety around accessing data, not allocating objects. To be clear why I was asking for examples, I don't have any problem not_null, but I do think references are better than not_nulls in most cases and am responding to "In many such cases, we may want to avoid needing to handle the pointer being null, or make it clear in the function contract that we require a non-null pointer" in the issue description. |
So you are saying there should be a |
I think the point of references is that they are transparent aliases to data somewhere that you should just access and not try to do memory management with. So if you are writing a function that just needs to access data |
I can see how references and reference_wrapper (which I didn't know was zero-cost until today) can solve most of the cases. I do think that |
This seems pretty niche, but I like the idea of being extra expressive when possible. I agree with @ryanofsky that references should be generally preferred though. I think @dongcarl pointed to a concrete example of where passing a reference or I'm curious though, is this better than using the nonnull attribute when needed? In terms of expressiveness, it seems to me that it might be more useful to peg the property to a specific parameter and locale than a storage duration. It wouldn't be correct-by-construction that way, but we could still get warnings/errors. So I guess my question is: is it more useful to enforce as part of the function signature, or storage duration? |
I'm curious about this, but I don't see what it is referring to. It seems like inheritance should be orthogonal to nullability and which pointer or reference mechanism is used.
It seems like ideal thing would be for the On larger issue I definitely think |
In any case, I think having not_null for |
The util::ResultPtr class is a wrapper for util::Result providing syntax sugar to make it less awkward to use with returned pointers. Instead of needing to be deferenced twice with **result or (*result)->member, it only needs to be dereferenced once with *result or result->member. Also when it is used in a boolean context, instead of evaluating to true when there is no error and the pointer is null, it evaluates to false so it is straightforward to determine whether the result points to something. This PR is only partial a solution to bitcoin#26004 because while it makes it easier to check for null pointer values, it does not make it impossible to return a null pointer value inside a successful result. It would be possible to enforce that successful results always contain non-null pointers by using a not_null type (bitcoin#24423) in combination with ResultPtr, though.
The util::ResultPtr class is a wrapper for util::Result providing syntax sugar to make it less awkward to use with returned pointers. Instead of needing to be deferenced twice with **result or (*result)->member, it only needs to be dereferenced once with *result or result->member. Also when it is used in a boolean context, instead of evaluating to true when there is no error and the pointer is null, it evaluates to false so it is straightforward to determine whether the result points to something. This PR is only partial a solution to bitcoin#26004 because while it makes it easier to check for null pointer values, it does not make it impossible to return a null pointer value inside a successful result. It would be possible to enforce that successful results always contain non-null pointers by using a not_null type (bitcoin#24423) in combination with ResultPtr, though.
The util::ResultPtr class is a wrapper for util::Result providing syntax sugar to make it less awkward to use with returned pointers. Instead of needing to be deferenced twice with **result or (*result)->member, it only needs to be dereferenced once with *result or result->member. Also when it is used in a boolean context, instead of evaluating to true when there is no error and the pointer is null, it evaluates to false so it is straightforward to determine whether the result points to something. This PR is only partial a solution to bitcoin#26004 because while it makes it easier to check for null pointer values, it does not make it impossible to return a null pointer value inside a successful result. It would be possible to enforce that successful results always contain non-null pointers by using a not_null type (bitcoin#24423) in combination with ResultPtr, though.
The util::ResultPtr class is a wrapper for util::Result providing syntax sugar to make it less awkward to use with returned pointers. Instead of needing to be deferenced twice with **result or (*result)->member, it only needs to be dereferenced once with *result or result->member. Also when it is used in a boolean context, instead of evaluating to true when there is no error and the pointer is null, it evaluates to false so it is straightforward to determine whether the result points to something. This PR is only partial a solution to bitcoin#26004 because while it makes it easier to check for null pointer values, it does not make it impossible to return a null pointer value inside a successful result. It would be possible to enforce that successful results always contain non-null pointers by using a not_null type (bitcoin#24423) in combination with ResultPtr, though.
The util::ResultPtr class is a wrapper for util::Result providing syntax sugar to make it less awkward to use with returned pointers. Instead of needing to be deferenced twice with **result or (*result)->member, it only needs to be dereferenced once with *result or result->member. Also when it is used in a boolean context, instead of evaluating to true when there is no error and the pointer is null, it evaluates to false so it is straightforward to determine whether the result points to something. This PR is only partial a solution to bitcoin#26004 because while it makes it easier to check for null pointer values, it does not make it impossible to return a null pointer value inside a successful result. It would be possible to enforce that successful results always contain non-null pointers by using a not_null type (bitcoin#24423) in combination with ResultPtr, though.
There are many places in the codebase where we use a pointer type (
*
,shared_ptr
,unique_ptr
, etc.) because of historical reasons (CBlockIndex*
) or for polymorphism. In many such cases, we may want to avoid needing to handle the pointer being null, or make it clear in the function contract that we require a non-null pointer.not_null<T>
is the solution proposed by the C++ Core Guidelines (relevant section), and restricts pointers and smart pointers to hold non-null values.Since I've learned about this, I've repeatedly ran into scenarios where I wish I could use it so as to make call semantics clearer and avoid misuse / unnecessary null handling.
There is a header-only implementation of
not_null<T>
here, however, there are two issues we need to solve:Expects
andEnsures
macros defined here, do we also import these or can we tweak our existing assertion macros to work?strict_not_null<T>
, which requires explicit construction. Do we need that or justnot_null<T>
?The text was updated successfully, but these errors were encountered: