[boost] Re: Idea: Automatic Objects

From: scleary_at_[hidden]
Date: 1999-12-29 09:08:58

> Here are some code review comments. These are not comments on the concept
of
> auto_action, but on a few implementation details.
>
> --------
>
> Consider the assignment operator:
>
>     // assignment operator: transfers ownership
>     auto_action & operator=(auto_action & Other)
>     {
>         if (&Other != this)
>         {
>             SafeAction();
>             Ptr() = Other.release();
>             FunctionObject() = Other.FunctionObject();
>         }
>         return *this;
>     }
>
> It is not exception safe. If the assignment operator for FunctionObject()
> throws an exception, then the object is left in an inconsistent state.
>
> A variation on the non-throwing swap idiom might work here.
> Something like:
>
>     // assignment operator: transfers ownership
>     auto_action & operator=(auto_action & other)
>     {
>         if (&Other != this)
>         {
>             FunctionObject copy(other.FunctionObject());
>             SafeAction();
>             Ptr() = other.release();
>             using std::swap;
>             swap(FunctionObject(), copy);
>         }
>         return *this;
>     }
>
To avoid this problem, I've placed "no-throwing" restrictions on
FunctionObject construction/destruction/assignment.  So I assume assign
can't throw, and you assume swap won't throw.  So either we say FO can't
throw on assign or FO can't throw on swap.

I _can_ see how that idiom can be useful in other cases, but here the more
stringent requirements on FO also protect the constructor -- it is implied
that the constructor of an auto_action cannot throw an exception; it
_always_ succeeds.  This can be a very useful property when analyzing the
stability of a section of code.

> --------
>
> There's a lot of use of "(void)" for parameter lists. Since it's C++, I
> suggest you use "()" instead.
>
OK, I'll change this one -- stylistic convention. . .  I do prefer to use
"(void)" for overloaded operators, though.

> --------
>
> You should remove these declarations:
>
>     // disable const copy and assignment
>     auto_action(const auto_action &);
>     auto_action & operator=(const auto_action &);
>
> The presence of the public non-const copy and assignment operators
prevents
> the compiler from generating the default ones. The declarations do no
good,
> and they may turn the compile time error of trying to copy a const
> auto_action into a link time error instead.
>
Good point.  I'll change this, too.

> --------
>
> It seems a little strange to have action() return a const reference to the
> action stored in the object. How about returning a copy instead?
>
Why?  What if the function object is large?  Most of the time it would be no
bigger than just a single pointer, but I've used larger ones.  The semantics
are the same (except for the number of temporary objects):

Return-by-const-reference ("const T & func();")
  T x(func()); // initializes x with the object referred to by func().  No
temporaries.
  const T & x = func(); // x refers to what func() referred to.  No
temporaries.

Return-by-value ("T func();")
  T x(func()); // initializes x with the return value of func.  One
temporary.
  const T & x = func(); // binds x to a temporary constructed from the
return value of func().  One temporary (not including the one bound to x).

The problem with return-by-const-reference is that the object must actually
be persistent in memory -- it cannot be a Mayfly (generated on-the-fly)
object.  So I find return-by-value useful in requirements specifications,
make a note that temporary objects required by return-by-value semantics may
not actually ever exist, and implement using return-by-const-reference
whenever possible.

> --------
>
> Why the "T * const" in these declarations?
>
>     explicit auto_action(T * const NPtr = 0,
>         const TFunctionObject & NFunctionObject = TFunctionObject())
>     void reset(T * const NPtr = 0)
>
> You should use "T *" instead.
>
No.  The "const" tells the compiler (and user) that we do not modify "NPtr"
in the constructor/reset method.

> --------
>
> I hope these comments are useful.
>
They have been; thanks!

        -Steve