Deleting from destructor

[mc]

Hi Experts,

This may be obvious but I can't find anything one way or another. We have
a class which is always allocated using new, e.g. Foo* foo = new Foo() so we
came up with the idea of releasing the memory from the destructor as
follows:

Foo::Foo()
{
// Initialize stuff
m_This = this; // m_This is a "void*"
}

Foo::~Foo()
{
// Release all resources
// and finally the memory
delete m_This;
}

It's been working fine so far (both Windows and Linux) but we're wondering
about it being either the worse thing to do..... Any thoughts.

Thank you.

Regards,

MC

[Victor Bazarov]

mc wrote:

This may be obvious but I can't find anything one way or another. We have
a class which is always allocated using new, e.g. Foo* foo = new Foo() so we
came up with the idea of releasing the memory from the destructor as
follows:

Foo::Foo()
{
// Initialize stuff
m_This = this; // m_This is a "void*"
}

Foo::~Foo()
{
// Release all resources
// and finally the memory
delete m_This;
}

It's been working fine so far (both Windows and Linux) but we're wondering
about it being either the worse thing to do..... Any thoughts.

That is a VERY BAD IDEA. Basically, the only correct way to invoke the
destructor of an object that was created dynamically is to use 'delete'
with the expression evaluating to the pointer to that object. In most
cases if you have

Foo* foo = new Foo();

somewhere, then somewhere else you have

delete foo;

That will invoke the destructor and *then* deallocate the memory. Now,
if you duplicate the pointer somewhere (in the object itself, or in some
other place), and then use 'delete' with that pointer, you will be
*deleting* the object twice. I am not sure what 'delete (void*)ptr'
does (or does not) compared to 'delete ptr'. Most likely it can't call
the destructor since the type is unknown. But it will, and I am certain
of it, deallocate the memory.

So, if you do use 'delete' with your 'foo', you're deallocating the
memory twice, which is undefined behaviour. If you don't use 'delete
foo', then how do you get the destructor to be called? DO you call it
explicitly (foo->~Foo();)? Then it's rather nonsensical, you should
instead use the normal idiomatic way and let the system perform all the
necessary clean-up for you.

V
--
Please remove capital 'A's when replying by e-mail
I do not respond to top-posted replies, please don't ask

[mc]

Thanks Victor. I understand what you said and knew. Let me put more
context here but adding a more complete example:

void SKEL::bind(const MCU& mcu, ...)
{
// const FOO& MCU::foo()
// {
// return (*new Foo());
// }
FOO foo = mcu.foo(); // Because of the const FOO&
returned, foo becomes equal to what was returned by MCU::foo()

// do stuff
// when exising here, the destructor for FOO is called and the memory is
release as per previous post
}

The destructor for the object is only called once and no memory leaks were
detected.

MC

"Victor Bazarov" <v.********@comAcast.netwrote in message
news:gc**********@news.datemas.de...

mc wrote:

>This may be obvious but I can't find anything one way or another. We
have a class which is always allocated using new, e.g. Foo* foo = new
Foo() so we came up with the idea of releasing the memory from the
destructor as follows:

Foo::Foo()
{
// Initialize stuff
m_This = this; // m_This is a "void*"
}

Foo::~Foo()
{
// Release all resources
// and finally the memory
delete m_This;
}

It's been working fine so far (both Windows and Linux) but we're
wondering about it being either the worse thing to do..... Any thoughts.

That is a VERY BAD IDEA. Basically, the only correct way to invoke the
destructor of an object that was created dynamically is to use 'delete'
with the expression evaluating to the pointer to that object. In most
cases if you have

Foo* foo = new Foo();

somewhere, then somewhere else you have

delete foo;

That will invoke the destructor and *then* deallocate the memory. Now, if
you duplicate the pointer somewhere (in the object itself, or in some
other place), and then use 'delete' with that pointer, you will be
*deleting* the object twice. I am not sure what 'delete (void*)ptr' does
(or does not) compared to 'delete ptr'. Most likely it can't call the
destructor since the type is unknown. But it will, and I am certain of
it, deallocate the memory.

So, if you do use 'delete' with your 'foo', you're deallocating the memory
twice, which is undefined behaviour. If you don't use 'delete foo', then
how do you get the destructor to be called? DO you call it explicitly
(foo->~Foo();)? Then it's rather nonsensical, you should instead use the
normal idiomatic way and let the system perform all the necessary clean-up
for you.

V
--
Please remove capital 'A's when replying by e-mail
I do not respond to top-posted replies, please don't ask

[Rolf Magnus]

mc wrote:

Thanks Victor. I understand what you said and knew. Let me put more
context here but adding a more complete example:

void SKEL::bind(const MCU& mcu, ...)
{
// const FOO& MCU::foo()
// {
// return (*new Foo());
// }
FOO foo = mcu.foo(); // Because of the const FOO&
returned, foo becomes equal to what was returned by MCU::foo()

Yes. It becomes a copy of it.

// do stuff
// when exising here, the destructor for FOO is called and the memory
is release as per previous post

The memory for the object foo in SKEL::bind is, but there is the other FOO
object that was dynamically allocated. It's still hanging around, and you
lost all pointers to it, so you can't ever deallocate it. That's a memory
leak. With the destructor you described, you also happen to use delete with
a pointer to memory you didn't get from new, which results in undefined
behavior.

}

The destructor for the object is only called once and no memory leaks were
detected.

Sounds like your memory debugger has a problem.

[mc]

I haven't lost the location where the object was; remember that in the
constructor a private member is initialized to the value of where the object
resides in memory; and the destructor uses a delete for that.
"Rolf Magnus" <ra******@t-online.dewrote in message
news:gc*************@news.t-online.com...

mc wrote:

>Thanks Victor. I understand what you said and knew. Let me put more
context here but adding a more complete example:

void SKEL::bind(const MCU& mcu, ...)
{
// const FOO& MCU::foo()
// {
// return (*new Foo());
// }
FOO foo = mcu.foo(); // Because of the const FOO&
returned, foo becomes equal to what was returned by MCU::foo()

Yes. It becomes a copy of it.

> // do stuff
// when exising here, the destructor for FOO is called and the memory
is release as per previous post

The memory for the object foo in SKEL::bind is, but there is the other FOO
object that was dynamically allocated. It's still hanging around, and you
lost all pointers to it, so you can't ever deallocate it. That's a memory
leak. With the destructor you described, you also happen to use delete
with
a pointer to memory you didn't get from new, which results in undefined
behavior.

>}

The destructor for the object is only called once and no memory leaks
were
detected.

Sounds like your memory debugger has a problem.

[Rolf Magnus]

Please don't top-post.

mc wrote:

"Rolf Magnus" <ra******@t-online.dewrote in message
news:gc*************@news.t-online.com...

>mc wrote:

>>Thanks Victor. I understand what you said and knew. Let me put more
context here but adding a more complete example:

void SKEL::bind(const MCU& mcu, ...)
{
// const FOO& MCU::foo()
// {
// return (*new Foo());
// }
FOO foo = mcu.foo(); // Because of the const FOO&
returned, foo becomes equal to what was returned by MCU::foo()

Yes. It becomes a copy of it.

>> // do stuff
// when exising here, the destructor for FOO is called and the
memory is release as per previous post

The memory for the object foo in SKEL::bind is, but there is the other
FOO object that was dynamically allocated. It's still hanging around, and
you lost all pointers to it, so you can't ever deallocate it. That's a
memory leak. With the destructor you described, you also happen to use
delete with
a pointer to memory you didn't get from new, which results in undefined
behavior.

I haven't lost the location where the object was; remember that in the
constructor a private member is initialized to the value of where the
object resides in memory; and the destructor uses a delete for that.

Yes, you're right. I missed that. So the copy's destructor frees the
original object's memory. However, the original's destructor isn't properly
called as it's supposed to, which probably won't lead to problems in the
example you showed, but will in less trivial programs. This is very bad
style.

[mc]

Why is there a need for the original's destructor to be called? After all,
the original is copied verbatim and consequently the destructor is called
once as expected.

Be that as it may, this is indeed quite obfuscated. The reasoning for
that is that the main caller does not know the object is being allocated
using new and hence is not supposed to call delete. So we had to find a way
to free the memory used and went for a new in place (see actual code below):

const Region& MCU::buffer(void) const
{
char* p = new char[sizeof(Region)];
return (*new(p) Region(std::string(TEMP), 1500));
}

with the Region ctdt:

Region::Region(const std::string& name, size_t size)
{
_this = reinterpret_cast<char *>(this);
_cache = new unsigned char[size], _offset = 0;
_fd = ::open(name.c_str(), O_RDWR | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR
| S_IRGRP | S_IROTH);
assert(_fd != -1);
}
Output::Buffer::~Buffer()
{
if (_fd != -1)
{
::write(_fd, (char *)_cache, _offset);
::close(_fd);
}
delete[] _cache;
delete[] _this;
}

Does that make sense?

[Kai-Uwe Bux]

You did it again. Please do not top-post. It is frowned upon around these
parts.

mc wrote:

Why is there a need for the original's destructor to be called? After all,
the original is copied verbatim and consequently the destructor is called
once as expected.

[snip]

I am not sure, I understand your code. You have:

Foo::Foo() {
// Initialize stuff
m_This = this; // m_This is a "void*"
}

Foo::~Foo() {
// Release all resources
// and finally the memory
delete m_This;
}

const FOO& MCU::foo() {
return (*new Foo());
}

and then

FOO foo = mcu.foo(); // creates a copy

Now the destructor of the foo object will be called. This destructor, in
turn, is going to invoke

delete m_This;

which will call the destructor for the original. Here is what I do not
understand:

a) Why is m_This a void*? It appears that in this case

delete m_This;

will _not_ invoke the destructor of the original.

b) Why are you not running into an infinite loop? After all, the destructor
of the original, once invoked, has to execute

delete m_This;

too.

c) Could it be that (a) and (b) are related, i.e., do you run into an
infinite loop if m_This had type foo*? If that is the case, then with the

delete m_This;

statement in the destructor of the copy foo, you are likely invoking
undefined behavior because of a type mismatch of the actual object and what
you are telling the compiler [5.3.5/3].
Best

Kai-Uwe Bux

[mc]

Sorry for being opaque but I have no idea what top-posting is....

[Ian Collins]

mc wrote:

Sorry for being opaque but I have no idea what top-posting is....

Ever heard of Google?

--
Ian Collins

[Default User]

mc wrote:

Sorry for being opaque but I have no idea what top-posting is....

<http://en.wikipedia.org/wiki/Top_posting>

See also:

<http://www.parashift.com/c++-faq-lite/how-to-post.html>


Brian

[mc]

"Default User" <de***********@yahoo.comwrote in message
news:6l************@mid.individual.net...

mc wrote:

>Sorry for being opaque but I have no idea what top-posting is....

<http://en.wikipedia.org/wiki/Top_posting>

See also:

<http://www.parashift.com/c++-faq-lite/how-to-post.html>


Brian

Thank you.

[Rolf Magnus]

mc wrote:

Why is there a need for the original's destructor to be called?

For the otherwise empty object you have here, it might be ok (I don't know
if it's sanctioned by the standard), but when your object gets more members
that are themselves class instances, you will run into trouble.

After all, the original is copied verbatim and consequently the destructor
is called once as expected.

But you have two objects. One constructed by the default constructor, the
other constructed by the copy constructor. And each of those two objects is
supposed to be constructed once and destroyed once.

Be that as it may, this is indeed quite obfuscated. The reasoning for
that is that the main caller does not know the object is being allocated
using new and hence is not supposed to call delete.

Why do you have the dynamically allocated object at all? Just return by
value, and you get the same, but in a clean way.

So we had to find a way to free the memory used and went for a new in
place (see actual code below):

const Region& MCU::buffer(void) const
{
char* p = new char[sizeof(Region)];
return (*new(p) Region(std::string(TEMP), 1500));
}

Why not simply:

const Region MCU::buffer(void) const
{
return Region(std::string(TEMP), 1500);
}