Platform SDK Shell, C++ ? related to shell programming

In my CShellFolder::CShellFolder() constructor method, I have the following
code:

if(FAILED(SHGetMalloc(&m_pMalloc)))
{
delete this;
return;
}

I also have similar instances of this code in the constructor as follows:

m_pPidlMgr = new CPidlMgr();
if(!m_pPidlMgr)
{
delete this;
return;
}

The goal, of course, is to cause construction of the object to fail if for
some reason the constructor can't allocate memory for data members that are
objects or if it fails to obtain the shell's memory allocator.

I have other code that then looks like this:

CShellFolder *pSF = new CShellFolder(constructor arguments);
if (!pSF)
return E_OUTOFMEMORY;


What I'm observing happen is that my CShellFolder class constructor method
has actually encountered a reason to fail, and it executes "delete this"
followed by "return", but the "new" operator is *NOT* returning a NULL
pointer. Instead, it appears to be returning the value of the "this"
pointer from the instance of the class that failed to be instantiated. I
would have expected it to return NULL.

This is the 1st time I've seen this class instantiation failure occur, and
the method of handling it is something I inherited from other code and it
looked like a reasonble method to use so I implemented it in my own code.

Is this in fact the wrong way to handle the situation?

Why would "new" return a non-NULL value if the object instance it was
creating fails in the constructor and performs "delete this"?


TIA,

Chuck
--
Chuck Chopp

ChuckChopp (at) rtfmcsi (dot) com http://www.rtfmcsi.com

RTFM Consulting Services Inc. 864 801 2795 voice & voicemail
103 Autumn Hill Road 864 801 2774 fax
Greer, SC 29651

Do not send me unsolicited commercial email.

In my IShellFolder implementation my memory allocator is on another object
that has static methods for retrieval...and the pidl manager is the same, so
I don't have to set up that stuff.

But it could be that an invalid pointer is being returned from the
CShellFolder constructor. I would look into seeing why its actually failing
and executing the "delete this" code. That for sure isn't a good thing.

I think its because of the way C++ handles class, dont quote me on this,
but im fairly sure it goes like this:

// CMyClass* pClass = new CMyClass();

pClass = malloc(sizeof(CMyClass));
if (pClass != NULL)
{
pClass->ctor(); // Call the constructor
}

Hence deleting the object in the contructor is no different from doing this:

CMyClass* pClass = new CMyClass();
delete pClass;

if (pClass != NULL)
{
// BANG!!
pClass->DoStuff();
}

Hope that helps

Aaaargh!!! More brain death inherited from sample code!!! This is really, *really* bad code and will cause undefined behaviour. Whoever wrote that code was clearly a C++ novice. If the constructor calls "delete this", then as you discovered, the new-expression returns a pointer to a deleted object - kaboom!

The only way a constructor can legally fail is by throwing an exception. If you don't want to do that, you need to use "two-phase construction", i.e. a minimal constructor that cannot fail followed by a normal member function that does the actual initialisation. (Other possibilities include using an out-parameter to return an error code or storing it in a member variable, but I wouldn't really recommend either of those.)

--
Jim Barry, MVP for Windows SDK

Wow Jim, I couldn't believe this either. It sent me off on a wild sanity
check into the pages of Stroustrup!!!!

He doesn't appear to say anything about this specifically (not in the places
I quickly checked) but I did find this that might be useful to confirm what
you've said to Chuck:

Page 128:

"The delete operator may be applied only to a pointer returned by new.
Applying delete to zero has no effect"

Anything else, I would have said, would cause at best undefined behaviour.
And in this case, that's exactly what is happening because the delete
happens in the constuctor before new has had a chance to return anything!!!!

Loz.

Thanks... It's oh so comforting to know that the sample code in question was
provided to me by Microsoft's shell developer support team in response to
me using up one of my support incidents on my MSDN subscription.

At 1st glance, the code looks elegant and appears that it would have the
desired effect. Upon further review, it's totally bogus. Makes me think of
those code snippet quizes you see on the envelope in which the C++
Developers' Journal subscription offers arrive in the mail.

I'll start throwing an exception if the constructor fails and wrap all calls
to "new" in exception trapping blocks.

And now I also need to review all of the drag-n-drop & clipboard handling
classes & code that were provided to me, too, for other unsavory coding
techniques.


I think it would be really helpful to have one complete namespace extension
sample project that makes use of current shell features [instead of stuff
from 6 years ago] and which follows best practices for C++ coding and for
NSE development. Having such a sample readily available might head off a
whole bunch of questions. It doesn't need to do anything fancy with MFC,
and it doesn't even need to rely on ATL *unles* there is a clear benefit to
doing so. What it does need to do is present a readily understandable
example of how a NSE really works and call attention to parts of the code
that deal with any of the more poorly documented COM interfaces.


--
Chuck Chopp

ChuckChopp (at) rtfmcsi (dot) com http://www.rtfmcsi.com

RTFM Consulting Services Inc. 864 801 2795 voice & voicemail
103 Autumn Hill Road 864 801 2774 fax
Greer, SC 29651

Do not send me unsolicited commercial email.

There's a piece of code I didn't include in my posting, and it's the one
resulting in the "delete this" being executed. It pertains to an issue
where some underlying data that represents a shell folder has somehow been
deleted, usually due to somebody messing around with the registry instead of
making proper changes through the application itself. Anyway, there's a
very valid situation I have to test for where the NSE's
IShellFolder::BindToObject() method is called to bind to a subfolder, but
the backing-store for the subfolder has been deleted and so I need to have
the construction of a new IShellFolder aborted and have BindToObject()
return a meaningful COM error code as a result of this situation.


--
Chuck Chopp

ChuckChopp (at) rtfmcsi (dot) com http://www.rtfmcsi.com

RTFM Consulting Services Inc. 864 801 2795 voice & voicemail
103 Autumn Hill Road 864 801 2774 fax
Greer, SC 29651

Do not send me unsolicited commercial email.

OK, let's say we're going with the constructor actually throwing an
exception in the event that *something* bad happens during object construction.

Would it then be proper to execute "delete this" immeiately before the
"throw" statement is executed?

I'm thinking it would be proper to do so since the outer scope level that
was using the "new" operator is now going to have to either catch the
exception or pass it up the stack, but the outer scope level shouldn't be
concerned with having to delete the object that failed construction. The
very nature of exception throwing & catching would seem to dictate that an
exception thrown in a constructor method should result in "delete this"
being executed. The end result would be that no further object cleanup
needs to be done. The object destructor would then have to be smart enough
to detect that the object only went through partial construction at the time
"delete this" was executed and so it would carefully delete any subordinate
class objects that may have been successfully created as class members.


--
Chuck Chopp

ChuckChopp (at) rtfmcsi (dot) com http://www.rtfmcsi.com

RTFM Consulting Services Inc. 864 801 2795 voice & voicemail
103 Autumn Hill Road 864 801 2774 fax
Greer, SC 29651

Do not send me unsolicited commercial email.

Never mind....

I think I got it figured out.

If an exception is caught in the constructor and then the same exception is
re-thrown or a new exception is thrown, the "new" operator ends up returning
NULL. What is interesting, though, is that the destructor is never called
as the "throw" statement immediately terminates the constructor; the memory
for the "this" pointer is deallocated, but object destruction never happens.

If "delete this" is executed in the "catch" block, then the "throw"
statement results in an assertion in one of the C++ RTL routines. This
makes it necessary to instead execute "this->~MyDestructor" just before the
"throw" statement since my class' destructor is written to properly
deallocate any objects that were created as class members. This also allows
for the global variable that counts DLL references to be properly
decremented as part of cleaning up after the failed object instantiation
attempt.


--
Chuck Chopp

ChuckChopp (at) rtfmcsi (dot) com http://www.rtfmcsi.com

RTFM Consulting Services Inc. 864 801 2795 voice & voicemail
103 Autumn Hill Road 864 801 2774 fax
Greer, SC 29651

Do not send me unsolicited commercial email.

Which compiler are you using? The "new" operator never returns null in standard C++ (unless the nothrow version is used). If an exception is thrown in the contructor, somebody has to catch it otherwise you have an unhandled exception.


Right, the object is not destructed because it never became fully constructed. Sub-objects (members and base classes) that were fully constructed will be destructed though. Therefore, any resource that needs to be subsequently freed should be managed by a member object.


Just don't hack about like this, it's bad. The C++ ctor/dtor model is designed to take care of these issues properly. The only legitimate use for "delete this" is for self-deleting objects, e.g. COM objects that delete themselves when the refcount goes to zero, or window objects that delete themselves when the window is destroyed.

I strongly recommend getting a copy Stroustrup's book "The C++ Programming Language". No serious C++ programmer should be without it. If you want to get in deeper, you can get a copy of the ISO C++ Standard too.

--
Jim Barry, MVP for Windows SDK

im Barry wrote:


I'm using the MS Visual C/C++ compiler that's part of Visual Studio .NET 2003.

I have a few instances where an exception can be thrown in the constructor,
but all of them are within a try...catch block. The "catch" block performs
cleanup and then re-throws the same exception to propagate it up the chain.
The code that was using the "new" operator also has the it's usage of
"new" inside a try...catch block where the exception gets trapped and
converted into a COM error such as E_OUTOFMEMORY or E_FAIL.


I'm trapping the exception and performing cleanup before propagating the
exception up the chain.

The subobjects in question are not members declared on the stack, such as
the follow:

class MyClass
{
public:

MyClass()
~MyClass()

private:

SomeOtherClass SomeObject;
};

Instead, I've got class members that are pointers to objects allocated from
the heap via "new".


private:

SomeOtherClass *pSomeObject;


and then:

MyClass::MyClass()
{
pSomeObject = new SomeOtherClass();
// The rest of my constructor continues here...
}


If an exception occurs in the constructor, and "pSomeObject" is not NULL,
then I need to perform "delete pSomeObject" before allowing the constructor
method to be terminated.


Right, I'm getting rid of all the occurrences of "delete this" that were
inside the constructors of the code that came from the CDBView sample project.

I understand that the constructor/destructor language elements are supposed
to take care of automatically handling destruction of subordinate objects
*when* those objects are stack-based objects declared as members of the
class itself. However, as soon as heap allocations with "new" or malloc()
are used, deconstruction of a partially constructed object becomes an issue.
Also, taking into account things like "++g_DllRefCount" in the constructor
and "--g_DllRefCount" in the destructor, deconstruction of a partially
constructed object is again important. In these exception situations where
the destructor method isn't called due to an exception terminating the
constructor method's execution, I absolutely have to take care of some
cleanup tasks, which I'm now handling as follows:

MyClass::MyClass()
{
try
{
// All constructor tasks go here.
}
catch (...)
{
Cleanup();
throw;
}
}

MyClass::~MyClass()
{
Cleanup();
}

void MyClass::Cleanup()
{
// Evaluate all class members that are pointers to
// objects allocated from the heap and perform a
// "delete" operation or a "->Release()" method call
// for shell-related COM objects.
};



I have a copy, but it's *OLD* - dated back to, oh, say, the very early
1990's. I will be getting an updated copy in the near future. I did a huge
amount of work with it prior to 1994, but had, shall we say, a long lapse in
time where clients permitted me to use C++ on various projects.


--
Chuck Chopp

ChuckChopp (at) rtfmcsi (dot) com http://www.rtfmcsi.com

RTFM Consulting Services Inc. 864 801 2795 voice & voicemail
103 Autumn Hill Road 864 801 2774 fax
Greer, SC 29651

Do not send me unsolicited commercial email.

Yes, and "smart pointers" are a good solution. See, for example:

http://www.boost.org/libs/smart_ptr/smart_ptr.htm


Yup, I started with the 2nd edition, still have it somewhere. The 3rd edition was a complete rewrite for standardised C++. There is a special edition available now, which is basically the 3rd edition in hardback with a couple of new appendices.

--
Jim Barry, MVP for Windows SDK