Challenging GotW 66's moral

[George2]

Hello everyone,


In GotW #66, one of the moral is the exception handler of constructor
should not do any like resource free task. I do not agree. Here is the
quoated moral and my code to prove this moral will have memory leak.

Anything wrong with my analysis?

http://www.gotw.ca/gotw/066.htm

Moral #1: Constructor function-try-block handlers have only one
purpose -- to translate an exception. (And maybe to do logging or some
other side effects.) They are not useful for any other purpose.

class A
{
private:

int* p;

public:

    A()
    try
    {
        p = new int[10];

        // there are some other exceptions here

    }
    catch (bad_alloc)
    {
        // do not delete since bad_alloc means memory pointed by p is
not allocated
    }
    catch (...)
    {
        // if we do not delete p, there will be memory leak
        // at this point, we are conflicting with Gotw 66's moral 1
        if (p) delete[] p;
    }
}

thanks in advance,
George

 

[Salt_Peter]

Hello everyone,

In GotW #66, one of the moral is the exception handler of constructor
should not do any like resource free task. I do not agree. Here is the
quoated moral and my code to prove this moral will have memory leak.

Anything wrong with my analysis?

http://www.gotw.ca/gotw/066.htm

Moral #1: Constructor function-try-block handlers have only one
purpose -- to translate an exception. (And maybe to do logging or some
other side effects.) They are not useful for any other purpose.

class A
{
private:

int* p;

public:

A()
try
{
p = new int[10];

// there are some other exceptions here

}
catch (bad_alloc)
{
// do not delete since bad_alloc means memory pointed by p is
not allocated
}
catch (...)
{
// if we do not delete p, there will be memory leak
// at this point, we are conflicting with Gotw 66's moral 1
if (p) delete[] p;[/QUOTE]

at this point, p would never be 0 (even if the new allocation in ctor
body was not processed yet).
Pointer p, as is, has either a valid address or garbage in it.
As far as that catch block is concerned, it'll always delete [] p.

So how about:

A() : p(0)
try
{
  p = new int[10];
}
catch(...)
{
  if (p) delete[] p;
}

have you considered shared_ptr instead of doing decadent new
allocations?

 

[Erik Wikström]

Hello everyone,


In GotW #66, one of the moral is the exception handler of constructor
should not do any like resource free task. I do not agree. Here is the
quoated moral and my code to prove this moral will have memory leak.

Anything wrong with my analysis?

http://www.gotw.ca/gotw/066.htm

Moral #1: Constructor function-try-block handlers have only one
purpose -- to translate an exception. (And maybe to do logging or some
other side effects.) They are not useful for any other purpose.

class A
{
private:

int* p;

public:

A()
try
{
p = new int[10];

// there are some other exceptions here

}
catch (bad_alloc)
{
// do not delete since bad_alloc means memory pointed by p is
not allocated
}
catch (...)
{
// if we do not delete p, there will be memory leak
// at this point, we are conflicting with Gotw 66's moral 1
if (p) delete[] p;
}
}

The advice assumes that you follow other advices, such as using RAII and
writing exception safe code, if you did you would never end up in a
situation where you need to free any memory in a catch-block. One
example of that would be to replace p with a smart-pointer.

 

[Daniel T.]

Hello everyone,


In GotW #66, one of the moral is the exception handler of constructor
should not do any like resource free task. I do not agree. Here is the
quoated moral and my code to prove this moral will have memory leak.

Anything wrong with my analysis?

Your idea is covered in the article:

"--But wait!" I hear someone interrupting from the middle of the
room. "I don't agree with Moral #1. I can think of another possible
use for constructor function-try-blocks, namely to free resources
allocated in the initializer list or in the constructor body!"

Sorry, nope. After all, remember that once you get into your
constructor try-block's handler, any local variables in the
constructor body are also already out of scope, and you are
guaranteed that no base subobjects or member objects exist any more,
period. You can't even refer to their names.

Maybe the output to the following will help:

class B {
public:
B() { cout << "B()\n"; }
~B() { cout << "~B()\n"; }
void foo() { cout << "B::foo()\n"; }
};

class A
{
private:
B b;
public:
A() try: b()
{
throw -1;
}
catch (...)
{
b.foo();
}
};

int main() {
try {
A a;
}
catch ( ... ) { }
}

Note that B::foo() is called *after b's destructor has already been
called.* Thus invoking undefined behavior.

http://www.gotw.ca/gotw/066.htm

Moral #1: Constructor function-try-block handlers have only one
purpose -- to translate an exception. (And maybe to do logging or some
other side effects.) They are not useful for any other purpose.

class A
{
private:

int* p;

public:

A()
try
{
p = new int[10];

// there are some other exceptions here

}
catch (bad_alloc)
{
// do not delete since bad_alloc means memory pointed by p is
not allocated
}
catch (...)
{
// if we do not delete p, there will be memory leak
// at this point, we are conflicting with Gotw 66's moral 1
if (p) delete[] p;[/QUOTE]

'p' doesn't exist once you get in the catch block. Yes, in your example 
it happens to still point to the right place, but there is no guarantee 
that this is true. Frankly, I'm surprised the code even compiled.
[QUOTE]
}
}

thanks in advance,
George

 

[Salt_Peter]

Hello everyone,

In GotW #66, one of the moral is the exception handler of constructor
should not do any like resource free task. I do not agree. Here is the
quoated moral and my code to prove this moral will have memory leak.

Anything wrong with my analysis?

Moral #1: Constructor function-try-block handlers have only one
purpose -- to translate an exception. (And maybe to do logging or some
other side effects.) They are not useful for any other purpose.

class A
{
private:[/QUOTE]
[QUOTE]
int* p; [QUOTE]
public:[/QUOTE]

A()
try
{
p = new int[10];[/QUOTE]
[QUOTE]
// there are some other exceptions here[/QUOTE]
[QUOTE]
}
catch (bad_alloc)
{
// do not delete since bad_alloc means memory pointed by p is
not allocated
}
catch (...)
{
// if we do not delete p, there will be memory leak
// at this point, we are conflicting with Gotw 66's moral 1
if (p) delete[] p;[/QUOTE]

at this point, p would never be 0 (even if the new allocation in ctor
body was not processed yet).
Pointer p, as is, has either a valid address or garbage in it.
As far as that catch block is concerned, it'll always delete [] p.

So how about:

A() : p(0)
try
{
p = new int[10];}

catch(...)
{
if (p) delete[] p;

}

have you considered shared_ptr instead of doing decadent new
allocations?[/QUOTE]


obviously, clarification is required since you can't allocate an array
using boost::shared_ptr.
replace the array with a std:vector or use boost::scoped_array.