remove null check during access and allow non-copyable pointer types like unique_ptr #650
Conversation
|
Updated pull request to include changes for supporting unique_ptr as discussed here: |
|
I'm not sure what the point of the static_assert that you could assign nullptr_t but it was blocking what seemed to be valid test cases, so I removed it. Also, the is_convertible was also stopping a better construction path so I updated that, as well. |
| @@ -68,10 +68,9 @@ using owner = T; | |||
| template <class T> | |||
| class not_null | |||
| { | |||
| public: | |||
| static_assert(std::is_assignable<T&, std::nullptr_t>::value, "T cannot be assigned nullptr."); | |||
There was a problem hiding this comment.
cppcoreguidelines says in http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#gslview-views the following about not_null<T : "T can be any type for which ==nullptr is meaningful.", so I guess it would make sense to static_assert on this kind of type trait instead : https://ideone.com/nuYfaB
|
Yes, that test would be correct. However, I think there's a certain
elegance to how small the code for not_null is and you still get a
compilation error without the static_assert, just not as nice of a
message. I'd rather not deal with deciding if introducting type traits in
<pointer> is the right thing to do during this PR. Removing the incorrect
static assert clearly makes not_null better, so I'd like to leave it like
that for now.
…--Zac
On Fri, Apr 13, 2018 at 3:45 AM, ericLemanissier ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In include/gsl/pointers
<#650 (comment)>:
> @@ -68,10 +68,9 @@ using owner = T;
template <class T>
class not_null
{
-public:
- static_assert(std::is_assignable<T&, std::nullptr_t>::value, "T cannot be assigned nullptr.");
cppcoreguidelines says in http://isocpp.github.io/CppCoreGuidelines/
CppCoreGuidelines#gslview-views the following about not_null<T : "T can
be any type for which ==nullptr is meaningful.", so I guess it would make
sense to static_assert on this kind of type trait instead :
https://ideone.com/nuYfaB
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#650 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAIyck700isaluCpWXDTPFTIbReIVLnvks5toIG7gaJpZM4TBRTC>
.
|
|
@xaxxon this PR makes two separate changes. Can you separate them out, so that we are not forced to take one to get the other? The part of the change that is easy to accept is removing the |
|
Do you know how to do that in github? I didn’t merge them intentionally it
just happened and you can’t have two forks of the same repo on GitHub from
what I can tell.
…On Mon, Apr 16, 2018 at 9:44 AM Neil MacIntosh ***@***.***> wrote:
@xaxxon <https://github.com/xaxxon> this PR makes two separate changes.
Can you separate them out, so that we are not forced to take one to get the
other? The part of the change that is easy to accept is removing the
Ensures (as discussed in the relevant issue). There is still some
discussion going on support for smart pointers.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#650 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAIycuGbR5Thrc1omdmQrE7QpE_hhmnxks5tpMpygaJpZM4TBRTC>
.
|
|
Also, my interest in putting more effort into this PR is directly tied to the interest this project puts into not breaking my existing code. I've spent a lot of time putting together benchmarks and such and am concerned that #659 is going to make me put a lot of time into adjusting my API to the point where I will be (again) deciding if I should just use my own fork. |
|
wrong button. |
@xaxxon You do one fork of the GSL repository, then create two (or more) branches in your fork. For each branch you can create a pull request. If you want so see this in action you can look at my fork https://github.com/beinhaerter/GSL with three branches (https://github.com/beinhaerter/GSL/branches), each for a different PR (https://github.com/Microsoft/GSL/pulls/beinhaerter). |
|
closing all my issues/PRs because GSL isn't at the maturity level of a library I can depend on. |
as discussed here: #649
the constructor/assignment operators already check that ptr_ is never set to null, so a runtime check during reads isn't necessary.
fixes #604 fixes #550 fixes #325 fixes #89 fixes #651