-
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
util::Result has confusing interface for std::*_ptr T #26004
Comments
cc @ryanofsky |
(For the mentioned bug, see bitcoin-core/gui#661 and fix in #26005) |
Good catch, and I think it would be good to do something to prevent this. Few ideas:
I tend to think (2) is nicest solution, but I'm not sure if there are other use-cases for non-nullable pointers that would justify the complexity since they don't seem completely trivial to implement. I don't like solution (1) because it creates an unnecessary difference between |
See also #24423 |
Forgot about that discussion. This does seem like a genuine use-case for Another idea for the list:
|
The issue with Can we maybe throw an exception in debug builds, but in production builds simply treat nullptr assignments as clearing? |
Would this impede having a constant |
This is a partial solution to bitcoin#26004. It makes it easier to avoid the mistake of assuming that just because a function returning `util::Result<unique_ptr>` succeeded, that the pointer it returns is also non-null. Using the `ResultPtr` return type, `if (result)` will only be true if the function was successful _and_ the result is non-null. This is not a complete solution to bitcoin#26004 because a more complete solution would use a non-nullable unique_ptr so a nullptr result is not possible at all. This change makes the nullptr state more straightforward to check for, but does not prevent it from occuring. Another motivation for using the `ResultPtr` class is to make calling pointer methods easier. For example in bitcoin#25722 if `wallet` is a `ResultPtr<unique_ptr<CWallet>>` it is possible to call `wallet->GetName()` instead of (*wallet)->GetName().
Yes, we would want to use a strict not_null implementation that makes it a compile error to try to assign a nullable pointer to a non-nullable one. Strict non_null pointers can only be initialized with special constructors, and if you call those constructors correctly, the compiler can guarantee the pointers are safe to dereference after that. The benefit of this is that it is easier to verify not_null pointers are constructed properly than it is to verify nullable pointers are safe to dereference every place they are dereferenced.
It would be an implementation detail of the not_null class, but it could provide a constructor that only throws in debug builds.
I'm not even sure we have this currently. |
We don't, but I think we should. It would be nice to put the abstraction interface between all accesses - trying to make CWallet itself abstract has proven impractical (trying to add support for a Lightning wallet). |
This is a partial solution to bitcoin#26004. It makes it easier to avoid the mistake of assuming that just because a function returning `util::Result<unique_ptr>` succeeded, that the pointer it returns is also non-null. Using the `ResultPtr` return type, `if (result)` will only be true if the function was successful _and_ the result is non-null. This is not a complete solution to bitcoin#26004 because a more complete solution would use a non-nullable unique_ptr so a nullptr result is not possible at all. This change makes the nullptr state more straightforward to check for, but does not prevent it from occuring. Another motivation for using the `ResultPtr` class is to make calling pointer methods easier. For example in bitcoin#25722 if `wallet` is a `ResultPtr<unique_ptr<CWallet>>` it is possible to call `wallet->GetName()` instead of (*wallet)->GetName().
This is a partial solution to bitcoin#26004. It makes it easier to avoid the mistake of assuming that just because a function returning `util::Result<unique_ptr>` succeeded, that the pointer it returns is also non-null. Using the `ResultPtr` return type, `if (result)` will only be true if the function was successful _and_ the result is non-null. This is not a complete solution to bitcoin#26004 because a more complete solution would use a non-nullable unique_ptr so a nullptr result is not possible at all. This change makes the nullptr state more straightforward to check for, but does not prevent it from occuring. Another motivation for using the `ResultPtr` class is to make calling pointer methods easier. For example in bitcoin#25722 if `wallet` is a `ResultPtr<unique_ptr<CWallet>>` it is possible to call `wallet->GetName()` instead of (*wallet)->GetName().
This is a partial solution to bitcoin#26004. It makes it easier to avoid the mistake of assuming that just because a function returning `util::Result<unique_ptr>` succeeded, that the pointer it returns is also non-null. Using the `ResultPtr` return type, `if (result)` will only be true if the function was successful _and_ the result is non-null. This is not a complete solution to bitcoin#26004 because a more complete solution would use a non-nullable unique_ptr so a nullptr result is not possible at all. This change makes the nullptr state more straightforward to check for, but does not prevent it from occuring. Another motivation for using the `ResultPtr` class is to make calling pointer methods easier. For example in bitcoin#25722 if `wallet` is a `ResultPtr<unique_ptr<CWallet>>` it is possible to call `wallet->GetName()` instead of (*wallet)->GetName().
This is a partial solution to bitcoin#26004. It makes it easier to avoid the mistake of assuming that just because a function returning `util::Result<unique_ptr>` succeeded, that the pointer it returns is also non-null. Using the `ResultPtr` return type, `if (result)` will only be true if the function was successful _and_ the result is non-null. This is not a complete solution to bitcoin#26004 because a more complete solution would use a non-nullable unique_ptr so a nullptr result is not possible at all. This change makes the nullptr state more straightforward to check for, but does not prevent it from occuring. Another motivation for using the `ResultPtr` class is to make calling pointer methods easier. For example in bitcoin#25722 if `wallet` is a `ResultPtr<unique_ptr<CWallet>>` it is possible to call `wallet->GetName()` instead of (*wallet)->GetName().
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.
This is a partial solution to bitcoin#26004. It makes it easier to avoid the mistake of assuming that just because a function returning `util::Result<unique_ptr>` succeeded, that the pointer it returns is also non-null. Using the `ResultPtr` return type, `if (result)` will only be true if the function was successful _and_ the result is non-null. This is not a complete solution to bitcoin#26004 because a more complete solution would use a non-nullable unique_ptr so a nullptr result is not possible at all. This change makes the nullptr state more straightforward to check for, but does not prevent it from occuring. Another motivation for using the `ResultPtr` class is to make calling pointer methods easier. For example in bitcoin#25722 if `wallet` is a `ResultPtr<unique_ptr<CWallet>>` it is possible to call `wallet->GetName()` instead of (*wallet)->GetName().
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.
util::Result<std::unique_ptr<...>>
treats a nullptr as atrue
value. This has caused at least one bug so far.Can we easily forbid passing nullptr in this way? Is there a better/safer solution?
The text was updated successfully, but these errors were encountered: