Fallible construction (even with a way to report failure) is annoying if
only because the object's destructor has to account for the possible
invalid states. I much prefer having a static creation method that will
only instantiate the object in case of success, and mark the constructor
protected. Something like:
static
already_AddRefed<Foo> Foo::Create(SomeParams...) {
// logic that may fail...
if (failed) {
return nullptr;
}
return MakeAndAddRef<Foo>(stuffThatDidNotFail);
}
Between having ways to communicate that an object is constructed in an
invalid state, and never having invalid states, I much prefer the
latter.
Cheers,
Nical
On Thu, Apr 21, 2016, at 07:05 AM, Gerald Squelart wrote:
> On Thursday, April 21, 2016 at 11:15:10 AM UTC+10, Nicholas Nethercote
> wrote:
> > Hi,
> >
> > C++ constructors can't be made fallible without using exceptions. As a
> > result,
> > for many classes we have a constructor and a fallible Init() method which
> > must
> > be called immediately after construction.
> >
> > Except... there is one way to make constructors fallible: use an |nsresult&
> > aRv| outparam to communicate possible failure. I propose that we start doing
> > this.
> >
> > Here's an example showing stack allocation and heap allocation. Currently,
> > we
> > do this (boolean return type):
> >
> > T ts();
> > if (!ts.Init()) {
> > return NS_ERROR_FAILURE;
> > }
> > T* th = new T();
> > if (!th.Init()) {
> > delete th;
> > return NS_ERROR_FAILURE;
> > }
> >
> > or this (nsresult return type):
> >
> > T ts();
> > nsresult rv = ts.Init();
> > if (NS_FAILED(rv)) {
> > return rv;
> > }
> > T* th = new T();
> > rv = th.Init();
> > if (NS_FAILED(rv)) {
> > delete th;
> > return rv;
> > }
> >
> > (In all the examples you could use a smart pointer to avoid the explicit
> > |delete|. This doesn't affect my argument in any way.)
> >
> > Instead, we would do this:
> >
> > nsresult rv;
> > T ts(rv);
> > if (NS_FAILED(rv)) {
> > return rv;
> > }
> > T* th = new T(rv);
> > if (NS_FAILED(rv)) {
> > delete th;
> > return rv;
> > }
> >
> > For constructors with additional argument, I propose that the |nsresult&|
> > argument go last.
> >
> > Using a bool outparam would be possible some of the time, but I suggest
> > always
> > using nsresult for consistency, esp. given that using bool here would be no
> > more concise.
> >
> > SpiderMonkey is different because (a) its |operator new| is fallible and
> > (b) it
> > doesn't use nsresult. So for heap-allocated objects we *would* use bool,
> > going
> > from this:
> >
> > T* th = new T();
> > if (!th) {
> > return false;
> > }
> > if (!th.Init()) {
> > delete th;
> > return false;
> > }
> >
> > to this:
> >
> > bool ok;
> > T* th = new T(ok);
> > if (!th || !ok) {
> > delete th;
> > return false;
> > }
> >
> > These examples don't show inheritance, but this proposal works out
> > straightforwardly in that case.
> >
> > The advantages of this proposal are as follows.
> >
> > - Construction is atomic. It's not artificially split into two, and there's
> > no
> > creation of half-initialized objects. This tends to make the code nicer
> > overall.
> >
> > - Constructors are special because they have initializer lists -- there are
> > things you can do in initializer lists that you cannot do in normal
> > functions. In particular, using an Init() function prevents you from using
> > references and |const| for some members. This is bad because references
> > and
> > |const| are good things that can make code more reliable.
> >
> > - There are fewer things to forget at call sites. With our current approach
> > you
> > can forget (a) to call Init(), and (b) to check the result of
> > Init(). With this
> > proposal you can only forget to check |rv|.
> >
> > The only disadvantage I can see is that it looks a bit strange at first.
> > But if
> > we started using it that objection would quickly go away.
> >
> > I have some example patches that show what this code pattern looks like in
> > practice. See bug 1265626 parts 1 and 4, and bug 1265965 part 1.
> >
> > Thoughts?
> >
> > Nick
>
> (busy right now, please excuse terseness & typos!)
>
> Big thumbs up for trying to remove split construction&inits.
>
> My main beef with this proposal is the use of out-params, which require
> (usually uninitialized) declaration of the out-param.
> But I see that it may indeed be the best solution here, so ... fiiiine!
>
> However, since lots of Mozilla objects are unique-ptr'd or ref-counted, I
> would argue that we could easily fold the construction checks with the
> nullptr-checks that we all know&love!
>
> So in addition to your proposal, I would like to see a small library of
> tools that will build on top of your new style, and make it easier &
> cleaner & consistent to use in those cases.
>
> E.g.:
> template <typename T, typename ...Args>
> T* newWithCheck(Args&&... aArgs)
> {
> nsresult rv;
> T* p = new T(std::forward<Args>(aArgs)..., &rv);
> if (p) { // <- this test could be removed for non-fallible new.
> if (NS_SUCCEEDED(rv)) {
> return p;
> }
> delete p; // "Failed" construction -> Just delete the thing.
> }
> return nullptr;
> }
>
> Then instead of:
> nsresult rv; // Yuck!
> RefPtr<Foo> foo = new Foo(a, b, &rv); // Hiss!
> if (NS_SUCCEEDED(rv)) { ... // Can we really trust foo here? Boo!
>
> We could do things like:
> RefPtr<Foo> foo = newWithCheck<Foo>(a, b); // Beauty!
> if (foo) { ... // Construction-check & nullptr-check in one test!
>
> Am I showing some bias? :-)
>
> We could have similar 'new' functions that would populate a Maybe<T>
> instead, or that would expose the nsresult where useful (e.g. through
> Pair<nsresult, RefPtr<T>>), etc.
>
>
> And of course, there's still the issue of missing checks.
>
> Your style probably helps, as the compiler and/or static analyzers should
> be able to see that you're defining 'rv', writing to it, but not reading
> it. If that's true, my proposal might mess with that. :-(
>
> So to combat that, could we create some new smart pointer type, that
> asserts that there is a nullptr check (through 'explicit operator
> bool()') before trying to dereference it? More thinking required...
> _______________________________________________
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
