gsl::not_null: Allow not_null (unique) pointers to be moved #1238
gsl::not_null: Allow not_null (unique) pointers to be moved #1238maflcko wants to merge 2 commits intomicrosoft:mainfrom
Conversation
carsonRadtke
left a comment
carsonRadtke
left a comment
There was a problem hiding this comment.
I'll bring this up in our next internal GSL sync, but my opinion is we don't want a type named "not_null" to ever possibly hold something that is null. (Next sync is April 15)
| EXPECT_EQ(nn_base.get(), | ||
| nullptr); // Warning: In real code, a use-after-move is not allowed! |
There was a problem hiding this comment.
This check violates F.23: Use a not_null to indicate that “null” is not a valid value (See the "Enforcement" section).
There was a problem hiding this comment.
Right, I think either way will violate some guidelines. About the enforcement: Comparing to nullptr is indeed the wrong approach. I am sure that clang-tidy use-after-move (https://clang.llvm.org/extra/clang-tidy/checks/bugprone/use-after-move.html) or other static analysers which enforce F18 ("Flag access to moved-from objects.") will flag this line in the test. I wanted to clarify this, but wasn't sure how to put it in test code.
Conversely, I think not allowing not_null<unique_ptr> will violate:
- https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#f18-for-will-move-from-parameters-pass-by-x-and-stdmove-the-parameter, because with taking
not_null<unique_ptr>won't compile - https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#f26-use-a-unique_ptrt-to-transfer-ownership-where-a-pointer-is-needed, because returning
not_null<unique_ptr>won't compile
So projects will have to pick an alternative and use runtime null checks, a raw pointer (violates https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#r3-a-raw-pointer-a-t-is-non-owning), or a shared pointer (violates https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#r21-prefer-unique_ptr-over-shared_ptr-unless-you-need-to-share-ownership)
I see that either approach has downsides and there is no ideal solution, but I think making not_null movable and using https://clang.llvm.org/extra/clang-tidy/checks/bugprone/use-after-move.html is the most pragmatic and practical one.
There was a problem hiding this comment.
I'm not following how any rules are violated.
F.18: For "will-move-from" parameters, pass by X&& and std::move the parameter
gsl::not_null cannot be a "will-move-from" parameter because it is not movable; there is no violation.
F.26: Use a unique_ptr to transfer ownership where a pointer is needed
gsl::not_null<unique_ptr<T>> is not the same as unique_ptr<T>. As the rule says: when you need ownership transfer, use unique_ptr.
So projects will have to pick an alternative and use ...
Or just don't use gsl::not_null when the underlying pointer might be null (as is the case after moving a unique_ptr).
I think making not_null movable and using [static analysis] is the most pragmatic and practical one.
I disagree with this, but I will raise it at next month's internal GSL sync.
There was a problem hiding this comment.
Ok, I see your view that there are no violations right now. So to summarize, there are two competing and likely incompatible views:
- Treat any movable unique pointer as nullable, due to the movability, in which case not_null does not apply and a plain
unique_ptrshould be used. (Probably the view held bygslright now) - Allow a movable unique pointer to be marked as not_null, and make the null-state impossible to observe with [static analysis].
I think there is also a third alternative: Add additional run-time checks when extracting or dereferencing the not_null, to also avoid observing a null-state at runtime. Happy to add those, but personally I think that'd be less clean than 1. or 2. above.
I guess in any case we'll have an answer to #991 after the next GSL sync call.
tests/pointers_tests.cpp
Outdated
| gsl::strict_not_null<std::unique_ptr<MyDerived>> nn_derived{std::make_unique<MyDerived>()}; | ||
| gsl::strict_not_null<std::unique_ptr<MyBase>> nn_base{std::make_unique<MyBase>()}; | ||
|
|
||
| #ifdef CONFIRM_COMPILATION_ERRORS |
There was a problem hiding this comment.
I guess we have precedent for this 'CONFIRM_COMPILATION_ERRORS' thing, but it isn't supported by our test system; let's avoid using it. For this one, you can assert on !std::is_copy{constructible,assignable}<T>::value.
There was a problem hiding this comment.
Thx, refactored the two CONFIRM_COMPILATION_ERRORS
carsonRadtke
added
Status: Review Needed
There is no need to call the not_null<T> constructor again, and call the check `Expects(ptr_ != nullptr);` again, because the check has already been called as part of not_null<U>.
2 participants
This fixes #991 by following the concept that gsl-lite is using:
After a move, dereferencing the not_null pointer is not allowed,because it will be in an invalid state (null). Users may use the clang-tidy rule
use-after-moveor otherwise must follow https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es56-write-stdmove-only-when-you-need-to-explicitly-move-an-object-to-another-scope to only callstd::moveto explicitly move a pointer into another scope, and not dereference the pointer after move.I understand that the prior design was done intentionally to avoid the null-state and tolerate a use-after-move. However, modern codebases that aim to adopt strict_not_null will normally also disallow
use-after-move, either via clang-tidy, or otherwise. Also, modern codebases often use patterns that require moving or extracting not-null pointers, which is not possible with the current version ofgsl::(strict_)not_null.