RAII- Am I making life difficult? [LONGISH]

[Nick Keighley]

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"

 

[Andre Kostur]

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.

 

[Alf P. Steinbach]

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

 

[Nick Keighley]

* Nick Keighley:

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.

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]

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

Inserting the same try/catch:

try
{ // Brace 1
} // 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.


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

 

[Andre Kostur]

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.

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.