Boost logo

Boost :

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


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