[boost] Re: Idea: Automatic Objects

From: Darin Adler (darin_at_[hidden])
Date: 1999-12-28 16:56:33

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;
    }

--------

There's a lot of use of "(void)" for parameter lists. Since it's C++, I
suggest you use "()" instead.

--------

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.

--------

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 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.

--------

I hope these comments are useful.

    -- Darin