not_null constructor is now explicit

[ericLemanissier]

solves #395

[beinhaerter]

Just curious: what's the reason to change the initialization in the tests and why have different initialization ("{}" vs. "()") in the same test?

[ericLemanissier]

it was just a way to check they all work. I can harmonize them if it is a blocking point

[beinhaerter]

OK, thanks, that was my guess.

[xaxxon]

I question the usefulness and desirability of this change as stated in the associated issue - especially as a breaking change.

[xaxxon]

an alternative would be to make a new type explicit_not_null or something if this behavior is desired.

[Amaroker]

I question the usefulness and desirability of this change as stated in the associated issue - especially as a breaking change.

Changing a parameter to not_null can be a breaking change, regardless if the constructor is explicit or not.

However, the explicit constructor is very useful because we don't want such changes to silently (until run-time) break something.

The sooner we have the explicit constructor, the better.

The core guidelines say:

• Prefer compile-time checking to run-time checking.

• Single-argument constructors should be declared explicit.

• Do not introduce implicit conversions (through conversion operators or non-explicit constructors) just to gain a minor convenience.

[xaxxon]

This breaks code which already uses notnull and it doesn't help at all in situations where it's already explicitly cast to a not null and then the source of the pointer changes its contract to allow null pointers, you're still going to get runtime failures. This feels very wrong to me for a type which is essentially an "adjective" not a "noun". This also stops you from directly hooking up a third party library which returns a pointer which contractually can't be null directly up to code that wants a not_null object. To do that would essentially require I re-write the non-explicit not_null code again to convert the pointer to a not_null pointer.

…

On Mon, Apr 16, 2018 at 3:35 PM Johan ***@***.***> wrote: I question the usefulness and desirability of this change as stated in the associated issue - especially as a breaking change. Changing a parameter to not_null can be a breaking change, regardless if the constructor is explicit or not. However, the explicit constructor is very useful because we don't want such changes to silently (until run-time) break something. The sooner we have the explicit constructor, the better. The core guidelines say: - Prefer compile-time checking to run-time checking. - Single-argument constructors should be declared explicit. - Do not introduce implicit conversions (through conversion operators or non-explicit constructors) just to gain a minor convenience. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#659 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIycsxD2xmb7Ml_qTlZLaLT5dIWDXB2ks5tpRyfgaJpZM4TTOPX> .

[ericLemanissier]

@neilmacintosh What do you think about this pull request ?

[neilmacintosh]

@ericLemanissier I personally like this change, for the reasons observed on the issue threads: not_null will frequently be used to make a function have a strong contract about null values, and it seems sensible to ask callers to explicitly acknowledge that contract.

I'll leave final approval to @annagrin though.

[annagrin]

@neilmacintosh Agreed. We do want to follow the Guidelines in GSL:)