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
