Boost logo

Boost :

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?


Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk