not_null has converting constructors?

[akrzemi1]

It seems to me that offering a conversion from T to not_null<T> looses the potential to detect certain bugs at compile-time.

if I have a function that returns a raw (potentially null) pointer, and I carelessly pass it to a function taking not_null it will compile fine, and will try report a bug at run-time when it is likely too late.

Instead, if the constructor from T were explicit, an inadvertent assignment:

use_ptr(make_ptr());

would be impossible, and I would be forced to explicitly require a potentially unsafe conversion:

use_ptr(not_null<T>{make_ptr()});

This would be a kind of the signature: by writing this cast, I am taking the responsibility for guaranteeing that the raw pointer will not be null. If it is not the case, you will know that I did it consciously.

See also CppCoreGuidelines issue: isocpp/CppCoreGuidelines#767

[Amaroker]

I agree with Andrzej.

It could at least be optional to force explicit conversions with something like:

#if defined(GSL_NOT_NULL_EXPLICIT_CONVERSION)

[viboes]

+1
It is up to the user to ensure that the pointer is not null.

In other words if had both not_null and not_null_explicit, I will use not_null_explicit.

[MikeGitb]

Whatever the solution: Please don't make its behavior dependent on some macro. I'm dreading the day when I include two external libraries, where each of them is relying on a different behavior.

[gdr-at-ms]

@MikeGitb Full agreed.

[MikeGitb]

There is a related PR from last year: #83

[neilmacintosh]

@akrzemi1 you make a good point. I think the principle to apply here is not whether or not we can catch more errors at compile-time. After all, the sorts of errors the compiler could catch this way are going to be relatively obvious, and the change would not substantively affect what other compile-time tooling could catch.

What convinces me here is a broader issue. If someone applies not_null to a parameter on an existing function, then they have effectively strengthened the requirements for calling that function. That should be a "breaking change" for callers - forcing them to ensure that their call sites meet the new contract. The best way to do that is to force the conversions to be explicit.

As @MikeGitb observes, there was a previous PR last year that got lost amidst conflicts with other changes to the GSL. I've closed that out as it is probably painful to revive at this point with all the file renames and so on that have occurred. Would you be interested in providing a new one?

[neilmacintosh]

@akrzemi1 I just noticed that you had provided a PR, sorry! It looks as though it's failing the automated checks at the moment...could you fix that up? Then I can take a look ;-)

[akrzemi1]

Neil, I know it would be decent of me to provide a complete patch, unit-tests included. I even tried it. But then the licensing process starts. It wants to know my home address, and I do not want to give such data to Microsoft.

In contrast, if I just talk to you, and convince you, and you implement it, it appears that this is fine from the legal point of view. I realize, it is a bit arrogant, but can we do it this way, without having Microsoft collecting my sensitive data?

[akrzemi1]

Also, the copy assignment from T has the same problem, and should probably be removed. If someone really needs it, they should be forced to type:

p = not_null<T>(t);

[neilmacintosh]

@akrzemi1 There's no need to convince me here, this is an issue with not_null (as someone pointed out earlier in the thread) that was already identified last year. Unfortunately that PR was obsolesced by other source changes before I could accept it.

I've replied on the PR explaining why I can't accept the PR as-is and why the CLA is a requirement for contributing.

[Amaroker]

This issue is holding us back for quite some time now. So, CLA is preventing progress, but how to move forward and fix it?

[neilmacintosh]

@Amaroker You could provide an alternate PR. :-) As I observed a while back, there was an earlier PR that was made obsolete by source movements, perhaps that one can be revived?

[galik]

I notice not_null is defined in <gsl/gsl> which also includes every other GSL header. Would this be a good time to address that? People who want not_null may not want everything else?

Looking at it, <gsl/gsl_util> only seems to have final_act in it so not_null could go in there?

Otherwise if <gsl/gsl_util> only contains one thing might it be better to rename that header to be more specific? Something like <gsl/gsl_final>?

The later you re-arrange things the more painful it will get. Although moving not_null to its own header could happen any time because <gsl/gsl> would continue to include it anyway (being that it is the catch-all header) so that shouldn't be a breaking change.

[Amaroker]

I'll consider it, but I'm new to github so I'll first need to learn how it works.

I once read Microsoft's CLA, but the jurisdiction/legal mumbo jumbo made me cringe. I'm not sure what it all means, nor am I familiar with the American laws referred to, or how my personal information will be used.

[neilmacintosh]

@galik this sounds like you have identified a useful issue, but probably unrelated to this one. do you want to open a separate issue so we don't lose track of your thoughts on header file structure with regard to not_null?

@Amaroker No problem. I am sure that someone will get to this eventually (perhaps even me). I was just suggesting that if this is a particular problem for you, you may want to contribute a fix. But, I completely understand that there are some hurdles to doing so ;-)

[galik]

@neilmacintosh done.

[martinmoene]

xref: issue #502: Relocate not_null and owner into <gsl/gsl_util>?

[xaxxon]

Maybe I'm misunderstanding, but making the constructor explicit doesn't seem to require any special sauce to sneak a nullptr into it:

https://godbolt.org/g/weA4nK

// pretend this is not_null
template<typename T>
struct C {
    explicit C(T);
};

struct S {
    C<int*> c;

    S(int * pi) :
        c(pi)
    {}
};


int main() {
    S(nullptr);
}

If I'm understanding it, I'm against this change.

[ericLemanissier]

on the line "c(pi)" (L15 in your godbolt link), you are constructing a not_null object from a raw pointer, so you consciously made the choice to ignore the possibility that the passed pointer is null. I don't see any way to be more "explicit" in case of object construction like this.
The fact that explicit constructor catches all implicit conversion in parameters passing is still relevant.

[xaxxon]

I should not be able to accidentally create a null not_null. Imagine the API for S changed from being a raw pointer to being a not_null (or in this case C). This code should throw an exception to let me know that I'm doing the wrong thing, not let it go because I did not make a conscious choice to ignore the possibility it was null when it wasn't allowed - in my hypothetical example, when I made my conscious decision, it was allowed.

Catching runtime errors due to programmer logic error is a significant part of what not_null is about.

[ericLemanissier]

Sure, but making constructors explicit does not change anything to runtime check, it just forces you to think about it when compilation fails

[xaxxon]

If you really need a free pass past the one-time on-creation null check, I'd suggest we make an additional type not_null_I_promise for which not_null has a special constructor for which bypasses the check.

Oh, I did misunderstand the original question.

[ericLemanissier]

It's not the point of this issue, nobody suggested changing the run time checks. The point here is just adding some compile time safeguards.

[xaxxon]

I am taking the responsibility for guaranteeing that the raw pointer will not be null.

That phrase in the original question is what threw me. What I guess he meant is that he's stating that he is aware that the pointer should not be null.

[xaxxon]

I'm not a fan of the change (assuming I now understand it). I want to be able to put a not_null somewhere in my code when I just want to add a runtime check (and know that I'm safe from there on in) and not push that knowledge to the caller necessarily with an API change. It would be trivial to make your own not_null wrapper that added this functionality, but you can't write something that would take it away (I don't think).

[ericLemanissier]

I fail to see where explicit constructor changes the situation you are describing. Maybe you have a precise example in mind ?

[xaxxon]

say my API has a function some_function(int *) and it doesn't dereference that pointer until a long time later and I decide I want to know now that it's not null. So I just cahnge my function to take a not_null<int*> instead. My caller never has to care. They may be determining that hte pointer is valid in some other way or getting the pointer from some other system.. They may know that the pointer is valid without having to think about whether it's null or not. I don't want to push that extra information to them necessarily.

[ericLemanissier]

In this case you don't have to change the signature of your function, you just check inside your function by creating a not_null from the parameter

[xaxxon]

Well, at this point making a breaking change would be quite unfortunate. A new type can be made with a different behavior if it's desirable.

And honestly I don't see it as being a very big win. It's more one of those "oh it's that compiler error again. I have to put a <copy>/<paste> thing around this to make it work. fine. Ok, now it's good" and when someone changes the behavior of the function whose return type you already wrapped in an explciit_not_null(some_func()) so that it can now return a null pointer, then it gets you absolutely nothing except more typing.

[ericLemanissier]

this issue can be closed, now that #659 is merged

[JordanMaples]

Maintainers' call: Closing as #659 is merged.