-
Notifications
You must be signed in to change notification settings - Fork 738
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
not_null no longer requires a copyable pointer type #675
Conversation
ceb8d64
to
bdf1bcc
Compare
allows support of supporting std::unique_ptr based on @xaxxon's work fixes microsoft#89 fixes microsoft#550 fixes microsoft#651
@ericLemanissier thanks! That looks clean and separate from the get() function change. How does |
0a243d8
to
39d4fa5
Compare
@annagrin I added to tests to make sure not_null<unique_ptr> is move constructible, move assignable, non copyable. Checking that the moved from not_null actually became null after move is currently not possible, because it triggers postcondition error in not_null::get, so I put this part of the test inside #ifdef GSL_THROW_ON_CONTRACT_VIOLATION. It could be simplified if #674 is merged |
tests/notnull_tests.cpp
Outdated
using NotNull1 = not_null<std::unique_ptr<UniquePointerTestStruct>>; | ||
|
||
// values are the same | ||
CHECK((NotNull1(std::make_unique<UniquePointerTestStruct>())->i == NotNull1(std::make_unique<UniquePointerTestStruct>())->i)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice also to check the value of field i
tests/notnull_tests.cpp
Outdated
CHECK(*dst.get() == 42); | ||
src = std::move(dst); | ||
#ifdef GSL_THROW_ON_CONTRACT_VIOLATION | ||
CHECK_THROWS_AS(!dst.get(), fail_fast); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we show that reset() and setting to nullptr do not compile?
// values are the same | ||
CHECK((NotNull1(std::make_unique<UniquePointerTestStruct>())->i == NotNull1(std::make_unique<UniquePointerTestStruct>())->i)); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be great to see how does this change behaves with other smart pointers
@ericLemanissier please ignore deleted comment, I misused unique_ptr. BTW, looks like your other change - removing Ensures() from get() - is incompatible with this one, as it will remove the null checking on moved
|
also, declared not_null move assignment as defaulted
@annagrin what do you think of this PR ? |
@ericLemanissier I would like to clear some points with this PR and #674.
|
A mitigation of the problem is that get returns a reference to const, so the unique_ptr cannot be reset, unless explicitely casting const away, which is not a good pratice. |
If you move from |
The moved from not_null contains an empty unique_ptr, so it is actually null. This is not a problem if the next operation is destruction, or assignment, which is a usual behaviour for moved from objects, and not surprising because it has the same behaviour as the class template argument type. |
@gdr-at-ms I could use your opinion on the semantic change - see my "1. Conceptual problem" above, @ericLemanissier If we accept this change, we would have access violations after move if #674 is checked in as well. So my inclination is to take this change but not the #674. |
@gdr-at-ms, @annagrin, what happens?
So I agree with @ericLemanissier that "This is not a problem if the next operation is destruction, or assignment, which is a usual behaviour for moved from objects". And there is no need in the Ensures() check in the get() in the Release build. |
Thanks @ericLemanissier , I am merging this change, but would need to reject the #674 - we cannot remove the null check now, because moved-from not-null smart pointer can now contain a null pointer. |
Maintainer's call: I've opened issue isocpp/CppCoreGuidelines#1499 to continue the discussion. |
allows support of supporting std::unique_ptr
based on @xaxxon's work
fixes #89
fixes #550
fixes #651
From the Core Guidelines:
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#note-61