The pattern seems reasonable enough.
DXR says we have 2579 "init" or "Init" functions, and 9801 callers of such
functions. I used:
function:init ext:cpp ext:h
callers:init ext:cpp ext:h
Do you propose we make an effort to fix up existing code, or just introduce
this as the preferred pattern in new code?
On Wed, Apr 20, 2016 at 6:07 PM, Nicholas Nethercote <n.netherc...@gmail.com
> 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
> _______________________________________________
> 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
