RAII- Am I making life difficult? [LONGISH]

Discussion in 'C++' started by Nick Keighley, Aug 3, 2005.

  1. Hi,

    In the beginning the code was:-

    BRUSHH new_brush = create_brush()
    BRUSHH old_brush = select_brush (new_brush)
    draw(...)
    select_brush (old_brush)
    destroy_brush (new_brush)

    Each function call is a wrapper around a C API call (don't worry
    this in't a question about the API). Any of these wrappers can
    throw. If it matters a new brush is created, then selected, the
    draw() call uses the brush, then everything is restored how it
    was. Obviously this code leaks resources like a sieve. But I
    didn't care if an error occured- it was reported and the program
    terminated. The OS could clean up the mess.

    But then the draw() call threw an error I wanted to recover
    from. This obviously would not do (just for starters new_brush
    would be lost). So I invoked RAII and wrote this:-

    Brush_RM new_brush = create_brush()
    // operator* returns a BRUSHH
    BRUSHH old_brush = select_brush (*new_brush)
    draw(...)
    select_brush (old_brush)
    // ~Brush_RM() calls destroy_brush(new_brush)

    Brush_RM is a resource manager class- a smart pointer [1].
    Obviously this is not much better (slightly worse?). If it throws
    in the draw() a destroyed brush will be used in the later code.
    So:-

    Brush_RM new_brush = create_brush() // throw on error
    Select_brush old_brush = select_brush (*new_brush) // throw on
    // error
    draw(...)
    // ~Select_brush() calls select_brush(old_brush)
    // ~Brush_RM() calls destroy_brush(new_brush)

    I havn't dealt with c-ctors or assignment (obviously I will have to).
    The functions called in the dtors must not throw. Since the
    original C-APIs don't throw this isn't hard. The ctors of the new
    classes *must* throw (or Bad Things will happen).

    So the code looks nice and neat and I havn't got any try-catch
    clauses. But I've introduced two new (but simple) classes. Have I
    avoided leaks? Am I making life too difficult? Just so you have a fair
    chance to shoot me down these are the classes:-

    class Brush_RM
    {
    public:
    Brush_RM(BRUSHH br): brush_(br) {}
    ~Brush_RM() { destroy_brush(brush_); // don't throw }
    BRUSHH operator*() { return brush_; }
    private:
    BRUSHH brush_;
    };

    class Select_brush
    {
    public:
    Select_brush(BRUSHH br): brush_(br) {}
    ~Select_brush() { select_brush(brush_); // don't throw }
    private:
    BRUSHH brush_;
    }

    I could continue fiddling with this. Heavens, I could actually try
    compiling it! But I figure I might learn something by just posting what
    I've got.


    [1] names: am I alone? I've never liked the term "auto_ptr()".
    Yeah right it's automatic- but what does it *do*? I'm probably way
    too late to be saying this... How come Boost have about four
    different sorts of "auto_ptr"?


    --
    Nick Keighley

    A ruby trembled. Two tourmaline nets failed to rectify the laser beam.
    A diamond noted the error. Both the error and the correction went into
    the general computer.
    Corwainer Smith "The Dead Lady of Clown Town"
    Nick Keighley, Aug 3, 2005
    #1
    1. Advertising

  2. Nick Keighley

    Andre Kostur Guest

    "Nick Keighley" <> wrote in
    news::

    > Hi,
    >
    > In the beginning the code was:-
    >
    > BRUSHH new_brush = create_brush()
    > BRUSHH old_brush = select_brush (new_brush)
    > draw(...)
    > select_brush (old_brush)
    > destroy_brush (new_brush)
    >
    > Each function call is a wrapper around a C API call (don't worry
    > this in't a question about the API). Any of these wrappers can
    > throw. If it matters a new brush is created, then selected, the
    > draw() call uses the brush, then everything is restored how it
    > was. Obviously this code leaks resources like a sieve. But I
    > didn't care if an error occured- it was reported and the program
    > terminated. The OS could clean up the mess.
    >
    > But then the draw() call threw an error I wanted to recover
    > from. This obviously would not do (just for starters new_brush
    > would be lost). So I invoked RAII and wrote this:-
    >


    Let me insert a try/catch for illustration purposes (and let's assume
    draw is throwing something derived from std::exception):

    try
    { // Brace 1

    > Brush_RM new_brush = create_brush()
    > // operator* returns a BRUSHH
    > BRUSHH old_brush = select_brush (*new_brush)
    > draw(...)
    > select_brush (old_brush)


    } // Brace 2
    catch (std::exception & e)
    {
    }

    > // ~Brush_RM() calls destroy_brush(new_brush)
    >
    > Brush_RM is a resource manager class- a smart pointer [1].
    > Obviously this is not much better (slightly worse?). If it throws
    > in the draw() a destroyed brush will be used in the later code.


    Hmm? Where? If draw() throws an exception, then the code between the
    point where the exception is thrown and Brace 2 is "skipped over". The
    only stuff that happens between those two points is that local variables
    go out of scope at their appropriate places. So whatever local variables
    draw() had constructed up to the point of the exception will be
    destroyed, as will new_brush (since it is passing out of scope at Brace
    2). The select_brush() function will not get called in this case...

    > So:-
    >

    Inserting the same try/catch:

    try
    { // Brace 1
    > Brush_RM new_brush = create_brush() // throw on error
    > Select_brush old_brush = select_brush (*new_brush) // throw on
    > // error
    > draw(...)

    } // Brace 2

    This time, the same local variables in draw() are destroyed, and
    old_brush and new_brush are destroyed _in that order_. So you would
    correctly "unselect" the new brush, and then you'd destroy it.

    > // ~Select_brush() calls select_brush(old_brush)
    > // ~Brush_RM() calls destroy_brush(new_brush)
    >
    > I havn't dealt with c-ctors or assignment (obviously I will have to).
    > The functions called in the dtors must not throw. Since the
    > original C-APIs don't throw this isn't hard. The ctors of the new
    > classes *must* throw (or Bad Things will happen).
    >
    > So the code looks nice and neat and I havn't got any try-catch
    > clauses. But I've introduced two new (but simple) classes. Have I
    > avoided leaks? Am I making life too difficult? Just so you have a fair
    > chance to shoot me down these are the classes:-
    >
    > class Brush_RM
    > {
    > public:
    > Brush_RM(BRUSHH br): brush_(br) {}
    > ~Brush_RM() { destroy_brush(brush_); // don't throw }
    > BRUSHH operator*() { return brush_; }
    > private:
    > BRUSHH brush_;
    > };
    >
    > class Select_brush
    > {
    > public:
    > Select_brush(BRUSHH br): brush_(br) {}
    > ~Select_brush() { select_brush(brush_); // don't throw }
    > private:
    > BRUSHH brush_;
    > }
    >
    > I could continue fiddling with this. Heavens, I could actually try
    > compiling it! But I figure I might learn something by just posting what
    > I've got.


    Looks pretty good. Personally I'd have the constructor of Select_brush
    be the one calling select_brush the first time around....:

    Select_brush::Select_brush(BRUSHH br) { brush_ = select_brush(br); };

    And when you instantiate it:

    Select_brush old_brush(*new_brush);


    > [1] names: am I alone? I've never liked the term "auto_ptr()".
    > Yeah right it's automatic- but what does it *do*? I'm probably way


    It automatically deletes the pointer it holds when its lifetime ends.

    > too late to be saying this... How come Boost have about four
    > different sorts of "auto_ptr"?


    Different ownership semantics. I haven't looked at boost specifically,
    but they probably have a shared ownership auto_ptr (AKA: reference
    counting), and an exclusive ownership (something closer to
    std::auto_ptr). You'd have to read on each of the boost::auto_ptrs to
    see what purpose each on serves.
    Andre Kostur, Aug 3, 2005
    #2
    1. Advertising

  3. * Nick Keighley:
    > In the beginning the code was:-
    >
    > BRUSHH new_brush = create_brush()
    > BRUSHH old_brush = select_brush (new_brush)
    > draw(...)
    > select_brush (old_brush)
    > destroy_brush (new_brush)
    >
    > Each function call is a wrapper around a C API call (don't worry
    > this in't a question about the API). Any of these wrappers can
    > throw. If it matters a new brush is created, then selected, the
    > draw() call uses the brush, then everything is restored how it
    > was. Obviously this code leaks resources like a sieve. But I
    > didn't care if an error occured- it was reported and the program
    > terminated. The OS could clean up the mess.


    Actually, if I'm not very much mistaken about which OS this is, the OS can
    also clean up the mess locally, sort of like a database rollback only it is
    on the canvas (OS "device context"). I'd look into that if I were you.
    It's interesting also from the viewpoint of wrapping it up in C++ classes.

    Btw., don't use all uppercase for ordinary names.

    Reserve that for macros.


    > But then the draw() call threw an error I wanted to recover
    > from. This obviously would not do (just for starters new_brush
    > would be lost). So I invoked RAII and wrote this:-
    >
    > Brush_RM new_brush = create_brush()
    > // operator* returns a BRUSHH
    > BRUSHH old_brush = select_brush (*new_brush)
    > draw(...)
    > select_brush (old_brush)
    > // ~Brush_RM() calls destroy_brush(new_brush)


    >
    > Brush_RM is a resource manager class- a smart pointer [1].
    > Obviously this is not much better (slightly worse?). If it throws
    > in the draw() a destroyed brush will be used in the later code.


    Again, if I'm not mistaken about which OS, an exception would invoke
    OS-level "undefined behavior", because a brush cannot be destroyed while
    selected, and the Brush_RM destructor would try to do exactly that.


    > So:-
    >
    > Brush_RM new_brush = create_brush() // throw on error
    > Select_brush old_brush = select_brush (*new_brush) // throw on
    > // error
    > draw(...)
    > // ~Select_brush() calls select_brush(old_brush)
    > // ~Brush_RM() calls destroy_brush(new_brush)
    >


    This isn't bad except that I'd like a readable & significant name like
    "AutoBrush" better than "Brush_RM".

    However.

    Instead of a Select_brush class, a Select_pen class and so forth, I'd go for
    a DrawingToolObjects class that can handle any number of drawing tool
    objects. Or the other way, I'd make the canvas that the objects are
    "selected" into an explicit object, not a global, and put some intelligence
    into that so that from a usage point of view one would not have to deal with
    this "selecting" at all. That's more complicated but perhaps worth it.


    PS: Where there isn't so great opportunity for reuse as above, you might
    want to look into Marginean & Alexandrescu's ScopeGuard class.

    --
    A: Because it messes up the order in which people normally read text.
    Q: Why is it such a bad thing?
    A: Top-posting.
    Q: What is the most annoying thing on usenet and in e-mail?
    Alf P. Steinbach, Aug 3, 2005
    #3
  4. Alf P. Steinbach wrote:
    > * Nick Keighley:


    > > In the beginning the code was:-
    > >
    > > BRUSHH new_brush = create_brush()
    > > BRUSHH old_brush = select_brush (new_brush)
    > > draw(...)
    > > select_brush (old_brush)
    > > destroy_brush (new_brush)
    > >
    > > Each function call is a wrapper around a C API call (don't worry
    > > this in't a question about the API). Any of these wrappers can
    > > throw. If it matters a new brush is created, then selected, the
    > > draw() call uses the brush, then everything is restored how it
    > > was. Obviously this code leaks resources like a sieve. But I
    > > didn't care if an error occured- it was reported and the program
    > > terminated. The OS could clean up the mess.

    >
    > Actually, if I'm not very much mistaken about which OS this is, the OS can
    > also clean up the mess locally, sort of like a database rollback only it is
    > on the canvas (OS "device context"). I'd look into that if I were you.


    I will. I suspected this wasn't the best way to solve the problems of
    the
    particular OS- I was more interested in the rolling back of two
    slightly
    entangled transactions. I initially tried to do it with one class but
    that got
    rapidly messy.


    > It's interesting also from the viewpoint of wrapping it up in C++ classes.


    yes, that's what I was interested in


    > Btw., don't use all uppercase for ordinary names.
    > Reserve that for macros.


    macros? Do C++ programs use macros? :)


    > > But then the draw() call threw an error I wanted to recover
    > > from. This obviously would not do (just for starters new_brush
    > > would be lost). So I invoked RAII and wrote this:-
    > >
    > > Brush_RM new_brush = create_brush()
    > > // operator* returns a BRUSHH
    > > BRUSHH old_brush = select_brush (*new_brush)
    > > draw(...)
    > > select_brush (old_brush)
    > > // ~Brush_RM() calls destroy_brush(new_brush)

    >
    > > Brush_RM is a resource manager class- a smart pointer [1].
    > > Obviously this is not much better (slightly worse?). If it throws
    > > in the draw() a destroyed brush will be used in the later code.

    >
    > Again, if I'm not mistaken about which OS, an exception would invoke
    > OS-level "undefined behavior", because a brush cannot be destroyed while
    > selected, and the Brush_RM destructor would try to do exactly that.


    I know, it was step on the way.

    > > So:-
    > >
    > > Brush_RM new_brush = create_brush() // throw on error
    > > Select_brush old_brush = select_brush (*new_brush) // throw on
    > > // error
    > > draw(...)
    > > // ~Select_brush() calls select_brush(old_brush)
    > > // ~Brush_RM() calls destroy_brush(new_brush)

    >
    > This isn't bad except that I'd like a readable & significant name like
    > "AutoBrush" better than "Brush_RM".
    >
    > However.
    >
    > Instead of a Select_brush class, a Select_pen class and so forth, I'd go for
    > a DrawingToolObjects class that can handle any number of drawing tool
    > objects. Or the other way, I'd make the canvas that the objects are
    > "selected" into an explicit object, not a global, and put some intelligence
    > into that so that from a usage point of view one would not have to deal with
    > this "selecting" at all. That's more complicated but perhaps worth it.


    hmm. wouldn't you still have to specify the tool you were using? Or
    would it
    be something like:-

    brush.draw(canvas, ...)
    pen.draw (canvas, ...)

    > PS: Where there isn't so great opportunity for reuse as above, you might
    > want to look into Marginean & Alexandrescu's ScopeGuard class.


    where would I look for this? (I know "Google is your friend"!)


    --
    Nick Keighley

    We recommend, rather, that users take advantage of the extensions of
    GNU C and disregard the limitations of other compilers. Aside from
    certain supercomputers and obsolete small machines, there is less
    and less reason ever to use any other C compiler other than for
    bootstrapping GNU CC.
    (Using and Porting GNU CC)
    Nick Keighley, Aug 3, 2005
    #4
  5. Andre Kostur wrote:
    > "Nick Keighley" <> wrote in
    > news::


    > > In the beginning the code was:-
    > >
    > > BRUSHH new_brush = create_brush()
    > > BRUSHH old_brush = select_brush (new_brush)
    > > draw(...)
    > > select_brush (old_brush)
    > > destroy_brush (new_brush)
    > >
    > > Each function call is a wrapper around a C API call (don't worry
    > > this in't a question about the API). Any of these wrappers can
    > > throw. If it matters a new brush is created, then selected, the
    > > draw() call uses the brush, then everything is restored how it
    > > was. Obviously this code leaks resources like a sieve. But I
    > > didn't care if an error occured- it was reported and the program
    > > terminated. The OS could clean up the mess.
    > >
    > > But then the draw() call threw an error I wanted to recover
    > > from. This obviously would not do (just for starters new_brush
    > > would be lost). So I invoked RAII and wrote this:-

    >
    > Let me insert a try/catch for illustration purposes (and let's assume
    > draw is throwing something derived from std::exception):
    >
    > try
    > { // Brace 1
    >
    > > Brush_RM new_brush = create_brush()
    > > // operator* returns a BRUSHH
    > > BRUSHH old_brush = select_brush (*new_brush)
    > > draw(...)
    > > select_brush (old_brush)

    >
    > } // Brace 2
    > catch (std::exception & e)
    > {
    > }
    >
    > > // ~Brush_RM() calls destroy_brush(new_brush)
    > >
    > > Brush_RM is a resource manager class- a smart pointer [1].
    > > Obviously this is not much better (slightly worse?). If it throws
    > > in the draw() a destroyed brush will be used in the later code.

    >
    > Hmm? Where? If draw() throws an exception, then the code between the
    > point where the exception is thrown and Brace 2 is "skipped over". The
    > only stuff that happens between those two points is that local variables
    > go out of scope at their appropriate places.


    and so new_brush is destroyed by Brush_RM's destructor. Since the
    second select_brush() is never called the now destroyed brush continues
    to be used by any subsequent draw() type code.

    > So whatever local variables
    > draw() had constructed up to the point of the exception will be
    > destroyed, as will new_brush (since it is passing out of scope at Brace
    > 2). The select_brush() function will not get called in this case...


    that's the problem...

    > > So:-
    > >

    > Inserting the same try/catch:
    >
    > try
    > { // Brace 1
    > > Brush_RM new_brush = create_brush() // throw on error
    > > Select_brush old_brush = select_brush (*new_brush) // throw on
    > > // error
    > > draw(...)

    > } // Brace 2
    >
    > This time, the same local variables in draw() are destroyed, and
    > old_brush and new_brush are destroyed _in that order_. So you would
    > correctly "unselect" the new brush, and then you'd destroy it.
    >
    > > // ~Select_brush() calls select_brush(old_brush)
    > > // ~Brush_RM() calls destroy_brush(new_brush)
    > >
    > > I havn't dealt with c-ctors or assignment (obviously I will have to).
    > > The functions called in the dtors must not throw. Since the
    > > original C-APIs don't throw this isn't hard. The ctors of the new
    > > classes *must* throw (or Bad Things will happen).
    > >
    > > So the code looks nice and neat and I havn't got any try-catch
    > > clauses. But I've introduced two new (but simple) classes. Have I
    > > avoided leaks? Am I making life too difficult? Just so you have a fair
    > > chance to shoot me down these are the classes:-
    > >
    > > class Brush_RM
    > > {
    > > public:
    > > Brush_RM(BRUSHH br): brush_(br) {}
    > > ~Brush_RM() { destroy_brush(brush_); // don't throw }
    > > BRUSHH operator*() { return brush_; }
    > > private:
    > > BRUSHH brush_;
    > > };
    > >
    > > class Select_brush
    > > {
    > > public:
    > > Select_brush(BRUSHH br): brush_(br) {}
    > > ~Select_brush() { select_brush(brush_); // don't throw }
    > > private:
    > > BRUSHH brush_;
    > > }
    > >
    > > I could continue fiddling with this. Heavens, I could actually try
    > > compiling it! But I figure I might learn something by just posting what
    > > I've got.

    >
    > Looks pretty good. Personally I'd have the constructor of Select_brush
    > be the one calling select_brush the first time around....:
    >
    > Select_brush::Select_brush(BRUSHH br) { brush_ = select_brush(br); };
    >
    > And when you instantiate it:
    >
    > Select_brush old_brush(*new_brush);


    I wondered about that but I wanted a "sanity check" on what I had so
    far. Was I completly lost?

    > > [1] names: am I alone? I've never liked the term "auto_ptr()".
    > > Yeah right it's automatic- but what does it *do*? I'm probably way

    >
    > It automatically deletes the pointer it holds when its lifetime ends.


    as I say, I'm *far* too late for this. I just never liked the name...

    > > too late to be saying this... How come Boost have about four
    > > different sorts of "auto_ptr"?

    >
    > Different ownership semantics. I haven't looked at boost specifically,
    > but they probably have a shared ownership auto_ptr (AKA: reference
    > counting), and an exclusive ownership (something closer to
    > std::auto_ptr). You'd have to read on each of the boost::auto_ptrs to
    > see what purpose each on serves.



    --
    Nick Keighley
    Nick Keighley, Aug 3, 2005
    #5
  6. Nick Keighley

    Andre Kostur Guest

    "Nick Keighley" <> wrote in
    news::

    > Andre Kostur wrote:
    >> "Nick Keighley" <> wrote in
    >> news::



    >> Hmm? Where? If draw() throws an exception, then the code between
    >> the point where the exception is thrown and Brace 2 is "skipped
    >> over". The only stuff that happens between those two points is that
    >> local variables go out of scope at their appropriate places.

    >
    > and so new_brush is destroyed by Brush_RM's destructor. Since the
    > second select_brush() is never called the now destroyed brush
    > continues to be used by any subsequent draw() type code.


    Ah, yes... you're right. That brush would still be "registered"
    somewhere within the drawing library, and you're risking it being used
    later on.

    >> > So:-
    >> >

    >> Inserting the same try/catch:
    >>
    >> try
    >> { // Brace 1
    >> > Brush_RM new_brush = create_brush() // throw on error
    >> > Select_brush old_brush = select_brush (*new_brush) // throw on
    >> > // error
    >> > draw(...)

    >> } // Brace 2
    >>
    >> This time, the same local variables in draw() are destroyed, and
    >> old_brush and new_brush are destroyed _in that order_. So you would
    >> correctly "unselect" the new brush, and then you'd destroy it.
    >>
    >> > // ~Select_brush() calls select_brush(old_brush)
    >> > // ~Brush_RM() calls destroy_brush(new_brush)
    >> >
    >> > I havn't dealt with c-ctors or assignment (obviously I will have
    >> > to). The functions called in the dtors must not throw. Since the
    >> > original C-APIs don't throw this isn't hard. The ctors of the new
    >> > classes *must* throw (or Bad Things will happen).
    >> >
    >> > So the code looks nice and neat and I havn't got any try-catch
    >> > clauses. But I've introduced two new (but simple) classes. Have I
    >> > avoided leaks? Am I making life too difficult? Just so you have a
    >> > fair chance to shoot me down these are the classes:-
    >> >
    >> > class Brush_RM
    >> > {
    >> > public:
    >> > Brush_RM(BRUSHH br): brush_(br) {}
    >> > ~Brush_RM() { destroy_brush(brush_); // don't throw }
    >> > BRUSHH operator*() { return brush_; }
    >> > private:
    >> > BRUSHH brush_;
    >> > };
    >> >
    >> > class Select_brush
    >> > {
    >> > public:
    >> > Select_brush(BRUSHH br): brush_(br) {}
    >> > ~Select_brush() { select_brush(brush_); // don't throw }
    >> > private:
    >> > BRUSHH brush_;
    >> > }
    >> >
    >> > I could continue fiddling with this. Heavens, I could actually try
    >> > compiling it! But I figure I might learn something by just posting
    >> > what I've got.

    >>
    >> Looks pretty good. Personally I'd have the constructor of
    >> Select_brush be the one calling select_brush the first time
    >> around....:
    >>
    >> Select_brush::Select_brush(BRUSHH br) { brush_ = select_brush(br);
    >> };
    >>
    >> And when you instantiate it:
    >>
    >> Select_brush old_brush(*new_brush);

    >
    > I wondered about that but I wanted a "sanity check" on what I had so
    > far. Was I completly lost?


    Nope, just a different style. You're way the select_brush would be more
    visible at the point of instantiation, my way it's a little more hidden
    within the Select_brush class.

    Something that you might want to consider as well, is having Select_brush
    only taking a Brush_RM instance instead of a raw BRUSHH. Depends on how
    your program all hangs together... may help to enforce that all of your
    BRUSHHs in your program are safely stored away in a smart pointer-ish
    object.
    Andre Kostur, Aug 4, 2005
    #6
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. pillbug
    Replies:
    1
    Views:
    529
    Alf P. Steinbach
    May 6, 2006
  2. Default User
    Replies:
    0
    Views:
    763
    Default User
    Oct 9, 2008
  3. Johannes Schaub (litb)

    Re: Why is RAII called RAII?

    Johannes Schaub (litb), Sep 12, 2010, in forum: C++
    Replies:
    2
    Views:
    390
    James Kanze
    Sep 18, 2010
  4. cpp4ever

    Re: Why is RAII called RAII?

    cpp4ever, Sep 12, 2010, in forum: C++
    Replies:
    1
    Views:
    397
    BGB / cr88192
    Sep 13, 2010
  5. Goran Pusic

    Re: Why is RAII called RAII?

    Goran Pusic, Sep 13, 2010, in forum: C++
    Replies:
    11
    Views:
    539
    ptyxs
    Sep 16, 2010
Loading...

Share This Page