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<unique_ptr<>> doesn't work #89

Open
trebconnell opened this Issue Sep 29, 2015 · 16 comments

Comments

Projects
None yet
9 participants
@trebconnell
Contributor

trebconnell commented Sep 29, 2015

Since guidelines specify: "not_null ... T can be any type for which ==nullptr is meaningful." this should work

@trebconnell

This comment has been minimized.

Contributor

trebconnell commented Sep 29, 2015

One additional sticking point here is what will get() return? Can't return the unique_ptr by value. I would say unique_pointer<>::pointer. How about for shared_ptr? That doesn't have a ::pointer typedef. Of course that can return by value, but would incur the cost of locking and incrementing.

@galik

This comment has been minimized.

Contributor

galik commented Sep 29, 2015

According to the guidelines for passing smart pointers it seems that the only reason we should be passing a smart pointer to a function is when that fuction modifies the ownership in some way. Otherwise, the recommendation is that we pass a raw pointer.

R.30: Take smart pointers as parameters only to explicitly express lifetime semantics

Reason: Accepting a smart pointer to a widget is wrong if the function just needs the widget itself.
It should be able to accept any widget object, not just ones whose lifetimes are managed by a particular kind of smart pointer.
A function that does not manipulate lifetime should take raw pointers or references instead.

Bearing that in mind I am not sure that not_null should be passing smart pointers into functions at all.

After all a function receiving a not_null generally wants to be able to make calls against the pointer without having to first check its validity. It is unlikely a function receiving a not_null would want to deal with its ownership and, even if it did, would we want to encourage that? After all such a function would have the power to reset the pointer breaking its not null contract.

Maybe there are use-cases I have missed but it seems to me the main use for the not_null (with respect to smart pointers) is for down-calling and therefore for passing only the raw pointer that the smart pointer is managing.

Therefore we might not want a not_null to contain a smart pointer, only its associated raw pointer.

To that effect we could create not_null specializations for both unique_ptr and shared_ptr whereby it captures their managed raw pointer using get() and implements its not null invariant on construction.

@kkoenig

This comment has been minimized.

Contributor

kkoenig commented Oct 1, 2015

The real question that needs to be asked here is should not_null<T> behave as an alias of T.

If so, not_null<T>::get should only exist if T::get exists. This would be especially interesting if it was implemented so that

decltype(std::declval<not_null<T>>().get()) == decltype(not_null<std::declval<T>().get()>)

With regards to unique_ptr, it would be ideal if we only supported compilers that support user-defined conversion to T&&, in the meantime we could get by only allowing implicit conversions to const T&

@kkoenig

This comment has been minimized.

Contributor

kkoenig commented Oct 1, 2015

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#-f17-use-a-not_nullt-to-indicate-that-null-is-not-a-valid-value

By using p.size(), this example indicates that (at least for array_view) not_null<T> behaves as an alias and functions are forwarded in this manner.

not_null<T*> check(T* p) { if (p) return not_null<T*>{p}; throw Unexpected_nullptr{}; }

void computer(not_null<array_view<int>> p)
{
    if (0<p.size()) {   // bad: redundant test
        // ...
    }
}
@neilmacintosh

This comment has been minimized.

Collaborator

neilmacintosh commented Oct 1, 2015

It looks to me that there are two different issues being discussed in this thread.

The first issue is whether or not not_null should contain its T, or just a reference to it. not_null is designed to contain its T. In the case of a raw pointer, that's pretty trivial and (hopefully) not controversial. For a class type, like unique_ptr or array_view, it should also be contained within the not_null, by value.

The second issue is whether not_null should forward functions for the T it contains. The answer here is yes. not_null is conceptually a wrapper that maintains an invariant for the T, but other than that, the usual behaviors and operations for the T should be available on not_null.

@Netrix

This comment has been minimized.

Netrix commented Jan 11, 2016

What if we want to differentiate invariant not_null that must always be not null from not_null argument that we wan't the function to get. This means that I would like to have sth like not_null_arg<pointer/smart_ptr> that is checked everytime it is created or transforment into that underlying variable is not null.

In other words I would like to be should that my std::shared_ptr or std::unique_ptr is not null when it is passed to any of my function:

void foo(not_null_arg<std::unique_ptr<Bar>> p_arg);

now if I am willing to get the unique_ptr out of arg I should be able to do std::move(p_arg.arg) out of it.
Unfortunately, using const& or & of not_null_arg doesn't really make sense since it actually may be null after taking the pointer out, but it can be checked via static analysis. I in this case matters the most for me is the ability to check that passed argument is not null and this can be done only in runtime while creating such object with its constructor that checks if it is null or not.

The class could be something like this.

template<class Type>
struct not_null_arg
{
  Type arg;

  not_null_arg(Type arg_)
    :  arg(std::move(arg_))     // of course with proper, efficient smart pointer handling instead of that
  {
       assert(arg != nullptr);  // this matters the most
  }
};
@gdr-at-ms

This comment has been minimized.

Member

gdr-at-ms commented Jan 11, 2016

See F.7 for passing pointers as arguments.

@Netrix

This comment has been minimized.

Netrix commented Jan 11, 2016

Ok, but what about the case where I want to use std::shared_ptr to pass shared ownership to a function and I want to make sure (from function POV) that it will not be empty?

@gdr-at-ms

This comment has been minimized.

Member

gdr-at-ms commented Jan 11, 2016

@Netrix
Sure. The issue is the frequency of such sceanrios, and whether that is better than expressing that statement as a precondition contract (yes, we don't have it yet but we might...). If it is better to express as contract, then maybe we might just as well invest the resource into getting that. If it is not, can you show realistic code scenario on both the production and consumption side -- e.g. what does it look like?

@Netrix

This comment has been minimized.

Netrix commented Jan 13, 2016

I think the initialization of injected objects would be good example for that. Consider many classes that have constructors or functions (setters) that accepts std::shared_ptr or std::unique_ptr that should be allways a valid pointer (not null). Eg. Foo(std::unique_ptr p_injectedObject). Since we want inject something to an object we are doing it via smart_ptr because this object should own it.

Now I am not aware of any mechanism that will allow me to be should that the pointer is not null, because passing a nullptr in such scenario is a nonsense so I would like to catch it as fast as possible.

This is why I would like to have sth like not_null for smart pointers since there is no other way (or I am not aware of) of doing it.

@gdr-at-ms

This comment has been minimized.

Member

gdr-at-ms commented Jan 13, 2016

@Netrix
That does not rule out contracts as an alternative, right?

@Netrix

This comment has been minimized.

Netrix commented Jan 14, 2016

If I understand correctly it doesn't but I don't know how to express them in code.

@kkeri

This comment has been minimized.

kkeri commented Feb 2, 2016

I'm experimenting with not_null<> and smart pointers, and run into similar issues like the ones discussed here.

Regarding unique_ptr, I see the problem as follows. unique_ptr<> is not copy-constructible, not_null<> is not move-constructible. not_null<unique_ptr<>> is neither copy- nor move-constructible. Further we can't move the enclosed unique_ptr<> without breaking the invariant.

It looks like a paradox, but intuitively speaking there is common ground. Just before a not_null<> goes out of scope, we could legally move from it, because there's no way to access it between the point of moving and the point of destruction (I hope this makes sense.) That's the case when not_null<> is returned from a function.

I wonder if C++ can distinguish this situation from the common case of moving from an object. Is it possible to express the distinction in the type system? If so, we could allow the former and disable only the latter.

@hsivonen

This comment has been minimized.

hsivonen commented Feb 15, 2017

FWIW, mozilla::NotNull works with smart pointers.

@ChrisChiasson

This comment has been minimized.

ChrisChiasson commented Mar 1, 2017

I cloned the GSL repo last night after reading the comments in gsl/gsl that not_null was meant to work with smart pointers. I had intended to use structures like std::vector<gsl::not_null<std::unique_ptr<T const>>>const& in the arguments to my functions, as there seems to be no "owning" version of std::reference_wrapper (std::unique_reference, anyone?). When compiling my functions, which attempt to dereference the pointers in the structure, I receive the error message: no match for ‘operator*’ (operand type is ‘const gsl::not_null<std::unique_ptr< .... In the mean time, I guess I'll just stick with regular std::unique_ptr. Any other suggestions? The people on this bug report seem to have given use cases like this some thought.

@neilmacintosh

This comment has been minimized.

Collaborator

neilmacintosh commented Mar 1, 2017

Hi @ChrisChiasson, thanks for your comment.

We are aware that it is possible to make a "not-null" wrapper type work with smart pointers. We are simply moving slowly and carefully here. The most common use-case we had for not_null is with raw pointers (which realy should be the most commonly passed type of pointer in most codebases). As a result, that was the first use-case addressed. I wanted to make sure we'd had some time to think through the details of handling smart pointers before we decided on exactly what that support would look like. I think we're pretty much at that point now. When I surface from the current standards committee meeting I'll take a look at resolving the smart pointer support issue.

ericLemanissier added a commit to ericLemanissier/GSL that referenced this issue May 2, 2018

not_null no longer requires a copyable pointer type
allows support of supporting std::unique_ptr
based on @xaxxon's work

fixes Microsoft#89
fixes Microsoft#550
fixes Microsoft#651

@ericLemanissier ericLemanissier referenced a pull request that will close this issue May 2, 2018

Open

not_null no longer requires a copyable pointer type #675

ericLemanissier added a commit to ericLemanissier/GSL that referenced this issue May 2, 2018

not_null no longer requires a copyable pointer type
allows support of supporting std::unique_ptr
based on @xaxxon's work

fixes Microsoft#89
fixes Microsoft#550
fixes Microsoft#651

ericLemanissier added a commit to ericLemanissier/GSL that referenced this issue May 2, 2018

not_null no longer requires a copyable pointer type
allows support of supporting std::unique_ptr
based on @xaxxon's work

fixes Microsoft#89
fixes Microsoft#550
fixes Microsoft#651

ericLemanissier added a commit to ericLemanissier/GSL that referenced this issue May 2, 2018

not_null no longer requires a copyable pointer type
allows support of supporting std::unique_ptr
based on @xaxxon's work

fixes Microsoft#89
fixes Microsoft#550
fixes Microsoft#651
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment