Re: [boost] Looking for thoughts on a new smart pointer: shared_ptr_nonnull

Subject: Re: [boost] Looking for thoughts on a new smart pointer: shared_ptr_nonnull
From: Gavin Lambert (gavinl_at_[hidden])
Date: 2013-10-07 02:49:52

On 10/7/2013 6:56 PM, Quoth Matt Calabrese:
> Again, if you can keep the system running in the case where you would be
> passing the null pointer, then it is your responsibility to do that.

I'm not disputing that.

> Your sanity check is the assert.

Which disappear in release builds, so are not sufficient for bugs that 
are not observed until after release.

> When did I say that I don't run tests on release builds? The point is
> you are knowingly introducing a completely separate code path that
> only occurs in release builds, that is only encountered as a part of
> UB

You were objecting to the code path not being exercisable in debug 
builds, and I pointed out that it doesn't matter as both paths are 
tested if you test both debug and release builds.  And while yes, it's a 
different code path, it is a trivial difference, and better than the 
alternative.  (Assert is itself a code path that is different between 
debug and release builds anyway, and it is common for iterators and 
memory allocators to have different code in debug builds, to say nothing 
of compiler optimisation, so it's hardly unusual.)

As for being encountered as part of UB, that is *only* if you decide to 
declare the constructor as having a non-null-parameter precondition by 
assumption instead of actually validating it.  Which is not the case in 
the position that I am arguing for, so that is irrelevant.

> No, it's not thrown before UB occurs. By having a function with
> preconditions and calling that function with values that don't meet those
> preconditions, you have undefined behavior. If you want specified behavior,
> then it's not a precondition. That said, I've already also explained why it
> should be a precondition. Please, let's not turn this into a vector at().

Can you please restate why you think it should be a precondition?  I've 
reread the thread and all I can find is "Initializing the object by
passing a null pointer should be UB since it is a clear violation of the
constructor's assumed precondition (don't pass a null pointer)" which is 
a statement with an assumption, not an explanation.  (You go on to quite 
some lengths about why throwing is bad for preconditions, but not why 
you think it should be a precondition in the first place.)

My argument is that it should be an explicitly-validated parameter, not 
an assumed-valid-via-precondition one.

And again, we're only talking about the constructor here.  I have no 
problem with operator-> etc assuming the invariant that the internal 
pointer is non-null, or that anything that accepts an 
already-constructed pointer can assume that the pointer is non-null.  I 
just don't agree that this applies to the constructor.

> An exception does not "avoid" anything. The mistake is still made and it is
> still a bug.

The exception avoids continuing on (in the case of a release build, 
where the assert is skipped) to construct an object with an invalid 
invariant and then execute further code that expects that invariant to 
be valid, which I hope we can all agree is well into unrecoverable UB 
territory.

The point is that at that instant all of the program's state, both 
global and local, is still undamaged.  (It may not be *happy* with 
having a null pointer, but until it tries to access it nothing undefined 
has yet happened, bar a quibble in documentation.)

There is a qualitive difference between the object throwing up its hands 
(aka an exception) and saying "you idiot, don't pass me a null pointer" 
vs. not even thinking about it and at some undefined point in the future 
(where it may be harder to track down the source) crashing or 
overwriting memory due to unchecked usage of a 
"never-supposed-to-be-null" pointer that secretly is actually null.

Again, in a perfect world these sorts of bugs will be caught in the 
debug build by the assert.  But we do not live in a perfect world, and 
test coverage is rarely 100%.

> I challenge you to define what you even mean by this. Do you mean that the
> code path is simply not common, and that this somehow makes things
> acceptable? There are plenty of branches in code that are not "normal."

I mean that the code path is never followed at all except in case of 
programmer error.  It's not merely "abnormal" or "rare" but "should 
never happen unless you have a bug", just like the assert itself.  Note 
that this last statement is NOT equivalent to "will never happen", which 
is what relying purely and only on asserts encourages.

When people object to using exceptions as control flow, this is not what 
they mean (because if you want to extend it that far, you're basically 
just saying "never use exceptions").

Would it help if you thought of it as "throw std::assertion_violation" 
in some version of "assert" that did not get compiled out for release 
builds?