"delete this" - Bad Code????

"delete this" - Bad Code????

Post by Paul Drummon » Thu, 02 Sep 1999 04:00:00



I won't bore you with an explination of my problem.  My question is:

Is it, GENERALLY, bad coding practise to "delete this"???

I do this statement at the very end of my function so I don't make any bad
calls.

 
 
 

"delete this" - Bad Code????

Post by Philip Stait » Thu, 02 Sep 1999 04:00:00


Generally?  IMHO yes it is bad coding practice.  Sure, it *is* safe and
useful, as long as certain conditions are met.  However, in *general*
relying on other users of your code (even yourself, say 6 mo down the
road) to remember and meet these conditions etc. is not a good idea.

 
 
 

"delete this" - Bad Code????

Post by Gene Bushuye » Thu, 02 Sep 1999 04:00:00


Why would you ever want to delete this? I don't see any legitimate reason
for such code.
this is only available inside member functions so this is the only place you
can call delete this. By doing so you run the risk of abnormal program
termination if your compiler uses "this" parameter in return code.

Gene Bushuyev


>I won't bore you with an explination of my problem.  My question is:

>Is it, GENERALLY, bad coding practise to "delete this"???

>I do this statement at the very end of my function so I don't make any bad
>calls.

 
 
 

"delete this" - Bad Code????

Post by Michael VanLoo » Thu, 02 Sep 1999 04:00:00



Quote:> Why would you ever want to delete this? I don't see any legitimate
reason
> for such code.
> this is only available inside member functions so this is the only
place you
> can call delete this. By doing so you run the risk of abnormal program
> termination if your compiler uses "this" parameter in return code.

There are very legitimate reasons for doing "delete this", however it
doesn't appear that Paul has provided any of them.  How would the
compiler use "this" in the parameter return code if you did not tell it
to?  If you code it safely, it works fine.

In general, yes, it is unnecessary, and if you're doing it, you should
evaluate if your design is flawed.  Most general-purpose cases would not
be correctly implemented with "delete this" anywhere in the code.

However, one example for a legitimate use of "delete this" is if you're
writing a reference-counted object.  In such an object, you should never
explicitely delete it -- you must always call "release" when you are
done using it (assuming an "addref" was done before hand).  When the
final release is called, and the reference count goes to zero, the
object is responsible for deleting itself.  How do you do that?  "delete
this".  An example of a safe implementation:

int RefCountClass::Release()
{
    int iRef = --m_iRefs;
    if (iRef == 0)
        delete this;
    return iRef; // note that m_iRefs may be no longer valid, so can't
be returned

}

> >I won't bore you with an explination of my problem.  My question is:

> >Is it, GENERALLY, bad coding practise to "delete this"???

> >I do this statement at the very end of my function so I don't make
any bad
> >calls.

 
 
 

"delete this" - Bad Code????

Post by Michael VanLoo » Thu, 02 Sep 1999 04:00:00



Quote:> The only case where I've had to use it is with a reference counting
garbage
> collection scheme. When the count is zero, the object is free to be
deleted,
> and the only way for an allocated object to delete itself is by using
> 'delete this'. This was for a set of classes that only I would be
using,
> so I didn't have to worry about what other users would be doing with

them.

My other post also pointed to reference-count situations as good
examples of this use.

Quote:> In short, it is generally bad practise, but given a potentially
troublesome
> situation, in C++ as in C, the assumption is that the programmer knows

best.

How can you say that reference-count situtations are "a potentially
troublesome situtation"?  They are one very good solution to a general
problem.  And they are used extensively in some little-known technology
called... what is it... COM?  Every Windows machine is running tens if
not hundreds or thousands of these objects.

The point is: it's a specific solution for a specific problem.  It's not
just a niche solution for odd troublesome situations.  BUT, it's not a
general solution that should be used unless you know exactly why you are
using it, and why another solution isn't better.


> > I won't bore you with an explination of my problem.  My question is:

> > Is it, GENERALLY, bad coding practise to "delete this"???

> > I do this statement at the very end of my function so I don't make
any bad
> > calls.

 
 
 

"delete this" - Bad Code????

Post by George Privalo » Thu, 02 Sep 1999 04:00:00


Myself being siner in that respect I must agree with your agrument that
"delete this" is potentially unsafe and very misleading practice.  At the
end, questionable benefits are outweighed by potential hasards and code that
is hard to understand.

George


>This neither necessary nor safe.
>Firstly, standard explicitly states that "this" pointer should contain
>the address of the object for which the member function is called.
>After "delete this" this rule is broken. You don't know what compiler
>can add at the end of your function call, especially if it employs
>debugging or profiling facilities. It well may end with fatal error.
>Secondly, there is no need to call delete from non-static member
>function. Static member function or external to this class function can
>do well.

>Gene Bushuyev

>Sent via Deja.com http://www.deja.com/
>Share what you know. Learn what you don't.

 
 
 

"delete this" - Bad Code????

Post by gb.. » Fri, 03 Sep 1999 04:00:00




[snip]

Quote:> However, one example for a legitimate use of "delete this" is if
you're
> writing a reference-counted object.  In such an object, you should
never
> explicitely delete it -- you must always call "release" when you are
> done using it (assuming an "addref" was done before hand).  When the
> final release is called, and the reference count goes to zero, the
> object is responsible for deleting itself.  How do you do that?
"delete
> this".  An example of a safe implementation:

> int RefCountClass::Release()
> {
>     int iRef = --m_iRefs;
>     if (iRef == 0)
>         delete this;
>     return iRef; // note that m_iRefs may be no longer valid, so can't
> be returned
> }

This neither necessary nor safe.
Firstly, standard explicitly states that "this" pointer should contain
the address of the object for which the member function is called.
After "delete this" this rule is broken. You don't know what compiler
can add at the end of your function call, especially if it employs
debugging or profiling facilities. It well may end with fatal error.
Secondly, there is no need to call delete from non-static member
function. Static member function or external to this class function can
do well.

Gene Bushuyev

Sent via Deja.com http://www.deja.com/
Share what you know. Learn what you don't.

 
 
 

"delete this" - Bad Code????

Post by Michael VanLoo » Fri, 03 Sep 1999 04:00:00





> > int RefCountClass::Release()
> > {
> >     int iRef = --m_iRefs;
> >     if (iRef == 0)
> >         delete this;
> >     return iRef; // note that m_iRefs may be no longer valid, so
can't
> > be returned
> > }
> Why not simply use:

> class RefCountClass {
> private:
>    int _refCount;
> protected:
>    int decrementRefCount() {
>         if (_refCount) return --_refCount; else return 0;

So you optimized this a little.  However, as a tangent, I think it's
horrible horrible horrible style to put all that *on one line.  It
is a rule that my developers are not allowed to put the if and its
operation on the same line.  Try this:

        if (_refCount)
            return --refCount;
        else
            return 0;

It will compile into the same code, and can actually be read without
having to rescan the line three times.  Remember: Source code is for
humans, not computers.

- Show quoted text -

Quote:>    }
>    int incrementRefCount() { return ++_refCount; }
> public:
>    RefCountClass() : _refCount(1) { }
>    static int Release(RefCountClass *me);
>    static int Bind(RefCountClass *me);
> }

> static int RefCountClass::Release(RefCountClass *me) {
>    int iRef = me->decrementRefCounter();
>    if (! iRef) delete me;
>    return iRef;
> }

> static int RefCountClass::Bind(RefCountClass *me) {
>    return me->incrementRefCount();
> }

> When you wish to decrement a reference, simply call the (safe) static
> RefCountClass::Release function on the reference.  The only drawback
> here is that someone could call decrementRefCounter() outside of the
> Release() function and possibly*up your reference count.  But
> that is the price you pay for trying to encapsulate referece counting
> behavior in the same class as the object being reference counted.

> A much better approach would be to move the reference counting
behaviour
> to an external object that acts as a Proxy for the object you are
> reference counting.  That way you get the behaviour you want with none
> of the * side effects.  Check out "Design Patterns" by Gamma

et.al.

Sure it's a solution.  One that puts more burden on the consumer of the
class, and you haven't proven to me that the original form is truly
unsafe.  So far, I've heard people say the compiler may, and it might do
something bad... but in my experience it doesn't.  So, until someone
quotes me something from the standard that says this behavior is
undefined, I do not consider it unsafe.

Plus, your proxy design has big drawbacks of its own in certain
situations.  Especially if the object behind the proxy is allowed to be
handled directly.  Then you wind up with the case where you can have two
or more proxy reference-count objects pointing to the same "real"
object, and when one of those proxies thinks you're finished with the
object and removes it, you suddenly have other proxies pointing to
invalid memory.  This can be much more insidious and harder to debug
than having the reference counting encapsulated directly into the class,
especially if the class was designed to work that way from the
beginning.

I'm not saying proxy patterns are bad -- they have very legitimate uses.
However I don't think they are the right approach for the case just
mentioned.

 
 
 

"delete this" - Bad Code????

Post by George W. Bayle » Fri, 03 Sep 1999 04:00:00


[snip]

Quote:> ... However, as a tangent, I think it's
> horrible horrible horrible style to put all that *on one line.  It
> is a rule that my developers are not allowed to put the if and its
> operation on the same line.  Try this:

>         if (_refCount)
>             return --refCount;
>         else
>             return 0;

Why not:

        return ( _refCount ) ? --refCount : 0;

But are we missing an underscore?

Quote:> It will compile into the same code, and can actually be read without
> having to rescan the line three times.  Remember: Source code is for
> humans, not computers.

Agreed.
 
 
 

"delete this" - Bad Code????

Post by Nick Ambros » Fri, 03 Sep 1999 04:00:00







> Sure it's a solution.  One that puts more burden on the consumer of the
> class, and you haven't proven to me that the original form is truly
> unsafe.  So far, I've heard people say the compiler may, and it might do
> something bad... but in my experience it doesn't.  So, until someone
> quotes me something from the standard that says this behavior is
> undefined, I do not consider it unsafe.

delete this is fine and safe as long as:

1.    Your class was allocated by new, not as an auto/global object etc.
2.    You don't call any non-static member functions through the deleted
pointer, or use any data member of the class.

If you can ensure (1) and (2) (I am of course probably forgetting or at
least oversimplifying things) then you are OK.

The problem with "in my experience" it doesn't is that "I just upgraded my
compiler and now all hell is breaking loose cause i am doing something
wrong" can easily happen.

Nick

 
 
 

"delete this" - Bad Code????

Post by Michael VanLoo » Fri, 03 Sep 1999 04:00:00





> [snip]
> > ... However, as a tangent, I think it's
> > horrible horrible horrible style to put all that *on one line.
It
> > is a rule that my developers are not allowed to put the if and its
> > operation on the same line.  Try this:

> >         if (_refCount)
> >             return --refCount;
> >         else
> >             return 0;

> Why not:

> return ( _refCount ) ? --refCount : 0;

Does that make it easier to read?  Does it actually make a measurable
performance difference in your program?  If both of these answers are
"no", then why do it (especially considering the point below)?

Quote:> But are we missing an underscore?

Why yes we are!  Good catch.

- Show quoted text -

Quote:> > It will compile into the same code, and can actually be read without
> > having to rescan the line three times.  Remember: Source code is for
> > humans, not computers.

> Agreed.

 
 
 

"delete this" - Bad Code????

Post by Michael VanLoo » Fri, 03 Sep 1999 04:00:00




> > Sure it's a solution.  One that puts more burden on the consumer of
the
> > class, and you haven't proven to me that the original form is truly
> > unsafe.  So far, I've heard people say the compiler may, and it
might do
> > something bad... but in my experience it doesn't.  So, until someone
> > quotes me something from the standard that says this behavior is
> > undefined, I do not consider it unsafe.
> delete this is fine and safe as long as:

> 1.    Your class was allocated by new, not as an auto/global object
etc.
> 2.    You don't call any non-static member functions through the
deleted
> pointer, or use any data member of the class.

> If you can ensure (1) and (2) (I am of course probably forgetting or
at
> least oversimplifying things) then you are OK.

And I did so.  Yes these are definitely common-sense constraints you
must live by if you go down this path.

However others were trying to say "the compiler may do X after your
delete without your knowledge".  And my answer was basically "prove it".

Quote:> The problem with "in my experience" it doesn't is that "I just
upgraded my
> compiler and now all hell is breaking loose cause i am doing something
> wrong" can easily happen.

I agree with you.  However "the compiler might do something bad" is just
as weak an argument, and I want to hear something more authoritative,
since this is a very useful pattern (and one which the Microsoft world
finds absolutely essential -- though that's also a weak precedent).
 
 
 

"delete this" - Bad Code????

Post by blarg » Sat, 04 Sep 1999 04:00:00






> > int RefCountClass::Release()
> > {
> >     int iRef = --m_iRefs;
> >     if (iRef == 0)
> >         delete this;
> >     return iRef; // note that m_iRefs may be no longer valid, so can't
> > be returned
> > }

> Why not simply use:

First, your class style (since you forced it on us)

Quote:> class RefCountClass {
> private:
>    int _refCount;

"_refCount" name is reserved for the implementation. Don't use a leading
underscore.

Quote:> protected:
>    int decrementRefCount() {
>         if (_refCount) return --_refCount; else return 0;

As someone else commented, this is needlessly terse.

    return _refCount ? --_refCount : 0;

if you want to be terse. Or, if performance is an issue, and you'll likely
be compiling for a modern architecture where branches are relatively
expensive, and your compiler isn't super-optimizing,

    return _refCount -= (_refCount != 0);

though this is only to show a sometimes-useful technique. I wouldn't
recommend it unless it has shown significant performance benefits.

Quote:>    }
>    int incrementRefCount() { return ++_refCount; }

Why are these protected? Exposing implementation details. Not nice for
your clients.

Quote:> public:
>    RefCountClass() : _refCount(1) { }
>    static int Release(RefCountClass *me);
>    static int Bind(RefCountClass *me);

Why do these return an int? What would one do with it? Exposing
implementation details again???

Quote:> }

> static int RefCountClass::Release(RefCountClass *me) {
>    int iRef = me->decrementRefCounter();
>    if (! iRef) delete me;
>    return iRef;
> }

> static int RefCountClass::Bind(RefCountClass *me) {
>    return me->incrementRefCount();
> }

Now you've forced a different functional style on us. Instead of
ref->Bind(), we have to do Bind( ref ). Member functions are more natural
in some cases, so why not use them? Wow, that's all member functions are
meant to be, an alternate syntax for these non-member functions (as well
as a mechanism for virtual binding). They should not involve any extra
"behind-the-scenes" junk. And that is the answer to this debate.

If we're going to discuss whether delete this is unsafe, I (and I imagine
others) are most interested in what the standard says about it being
defined. I don't care whether it's considered good style. That's for me to
decide in the particular situation I use it in (if I use it at all).

If you're going to talk about what the standard has to say, provide quotes
or section references. I don't care if you think it may not work. I want
references or nothing.

[snip]

 
 
 

1. "delete []" in place of "delete"

Hi,

Can I always use "delete []" in place of "delete"?  Both of the following
two code sequences work:

(1) int * i = new int;  delete i;
(2) int * i = new int;  delete [] i;

In what situation would "delete []" cause a compiling error?

Thank you,

Paihung

2. upgrade newbie questions

3. please tell me diffrent between "delete" and "delete[]"

4. migrating VB 6.0 w/ CE toolkit to VB.Net w/ SDE

5. Difference between "source code" and "object code."

6. Uses of Vector Quanitzation in image Compression

7. It's bad to use "delete"

8. Prodigy phone #

9. run time "new" and "delete"

10. "delete" operator and "auto_ptrs"

11. "own" vs "uses" vs "contains" vs "is a"

12. what is the Virtual Key Code of "[" and "]" ?