Prev: boolean
Next: asynchronous and synchronous call between apartments
From: Jim Johnson on 4 Apr 2008 21:30
is this C style coding?

I don't seem to see much C++ code in this way.

is this a bad programming practice?

code seem ugly coding this way.


=================
CATCHERROR(m_Cnn,0)

=================
#define CATCHERROR(ptr,a) catch(_com_error &e)\
{\
ErrorHandler(e,m_ErrStr);\
ptr=NULL;\
return a;\
}

From: Alex Blekhman on 5 Apr 2008 04:43
"Jim Johnson" wrote:
> is this C style coding?
>
> I don't seem to see much C++ code in this way.
>
> is this a bad programming practice?
>
> code seem ugly coding this way.
>
> =================
> CATCHERROR(m_Cnn,0)
>
> =================
> #define CATCHERROR(ptr,a) catch(_com_error &e)\
> {\
> ErrorHandler(e,m_ErrStr);\
> ptr=NULL;\
> return a;\
> }


There is not enough information to say conclusively whether this
code is ugly or not. Although generally macros are evil, still
there are situatons where macros may be helpful.

From the code you provided I can see that `m_ErrStr' variable is
used. It suggests that the `CATCHERROR' macro is only a part of a
bigger exception handling machinery. Also, it is unclear what's
the purpose of the `ptr' parameter of the macro. It seems that
other than being nullified it's not used. If it is some kind of
cleanup, then it's definitely ugly C-style. One should use
destructors to clean resources that can be leaked during an
exception.

It looks that the `CATCHERROR' macro is used on a COM module
boundary to catch internal exceptions and translate them into
return values. Then probably it would be wiser to return actual
HRESULT of an error code (retrievable via e.Error() call) instead
of arbitrary zero value.

Alex


From: Giovanni Dicanio on 5 Apr 2008 04:51

"Jim Johnson" <aopiyy001(a)yahoo.com> ha scritto nel messaggio
news:lgldv39oat9jm42ur9m2n974l4h6gr3cfn(a)4ax.com...

> =================
> CATCHERROR(m_Cnn,0)
>
> =================
> #define CATCHERROR(ptr,a) catch(_com_error &e)\
> {\
> ErrorHandler(e,m_ErrStr);\
> ptr=NULL;\
> return a;\
> }

The above is C++ code, because it uses 'catch' to catch exception
(_com_error &), and I believe it is not possible in pure C.

The CATCHERROR seems a preprocessor macro defined to avoid repeating
boiler-plate code.

Frankly speaking, if this macro is used wisely and not abused, I would not
define that bad code.
*Sometimes* preprocessor macros can come in handy.

e.g. I read some DirectShow SDK and COM code that uses HRCALL macro. It is
also available on MSDN.

<code>

// Macro that calls a COM method returning HRESULT value:
#define HRCALL(a, errmsg) \
do { \
hr = (a); \
if (FAILED(hr)) { \
dprintf( "%s:%d HRCALL Failed: %s\n 0x%.8x = %s\n", \
__FILE__, __LINE__, errmsg, hr, #a ); \
goto clean; \
} \
} while (0)

</code>

<code>

// Helper function that put output in stdout and debug window
// in Visual Studio:
void dprintf( char * format, ...)
{
static char buf[1024];
va_list args;
va_start( args, format );
vsprintf_s( buf, format, args );
va_end( args);
OutputDebugStringA( buf);
printf("%s", buf);
}

// Helper function to create a DOM instance:
IXMLDOMDocument * DomFromCOM()
{
HRESULT hr;
IXMLDOMDocument *pxmldoc = NULL;

HRCALL( CoCreateInstance(__uuidof(MSXML2::DOMDocument30),
NULL,
CLSCTX_INPROC_SERVER,
__uuidof(IXMLDOMDocument),
(void**)&pxmldoc),
"Create a new DOMDocument");

HRCALL( pxmldoc->put_async(VARIANT_FALSE),
"should never fail");
HRCALL( pxmldoc->put_validateOnParse(VARIANT_FALSE),
"should never fail");
HRCALL( pxmldoc->put_resolveExternals(VARIANT_FALSE),
"should never fail");

return pxmldoc;
clean:
if (pxmldoc)
{
pxmldoc->Release();
}
return NULL;
}
</code>


My 2 cents.

Giovanni


From: Giovanni Dicanio on 5 Apr 2008 11:10

"Alex Blekhman" <tkfx.REMOVE(a)yahoo.com> ha scritto nel messaggio
news:uSZWmkvlIHA.3512(a)TK2MSFTNGP03.phx.gbl...

> There is not enough information to say conclusively whether this code is
> ugly or not. Although generally macros are evil, still there are situatons
> where macros may be helpful.

I do agree.


> One should use destructors to clean resources

Yes, in general RAII can help in building more quality code.
And I agree we should use RAII when possible.

However, RAII (and related smart pointers) may also have their fine points
and subtle aspects, e.g. in this code sample:

<pseudo-code>

some function or method
{
CoInitialize...

ComSmartPointer sp1;
ComSmartPointer sp2;
...
ComSmartPointer spN;

...
use COM objects via their associated smart pointers
...

... Assume that smart pointer destructors call Release()

CoUninitialize();
}

</pseudo-code>

The above code is wrong, because I believe that the smart pointer
destructors are called just before exiting the code block, so *first*
CoUninitialize is called, *then* the smart pointer destructors are called.
And that is not correct (because I think that CoUninitialize must be called
*after* all COM interfaces are Release'd).

This is a subtle thing.

*Explicit* Release() with a 'goto cleanup' statement (like some MSDN and
other SDK documentations do) would have made the code flow more clear, and
avoid the above bug.

<pseudo-code>

... if (FAILED(hr) )
goto cleanup;

...
.... if (FAILED(hr))
goto cleanup;

...

cleanup:
// Explicitly call Release here
...

</pseudo-code>


And IMHO a preprocessor macro like CHECK_HR (that checks if current HRESULT
is a failure code and jumps to 'cleanup' label accordingly) would be good in
that scenario, too.

Of course, there are workarounds for the aforementioned bug, too, e.g.
enclose the smart pointers into a { ... } block, and put
CoInitialize/CoUninitialize out of that block:

<pseudo-code>

{
CoInitialize...
{
... smart pointers defintition and usage...
}
CoUninitialize();
}

</pseudo-code>


Giovanni



From: Alex Blekhman on 5 Apr 2008 13:06
"Giovanni Dicanio" wrote:
> However, RAII (and related smart pointers) may also have their
> fine points and subtle aspects, e.g. in this code sample:
>
> <pseudo-code>
>
> some function or method
> {
> CoInitialize...
>
> ComSmartPointer sp1;
> ComSmartPointer sp2;
> ...
> ComSmartPointer spN;
>
> ...
> use COM objects via their associated smart pointers
> ...
>
> ... Assume that smart pointer destructors call Release()
>
> CoUninitialize();
> }
>
> </pseudo-code>

Yes, this error is quite common. That's why I usually prefer to
make a simple class for COM initialization:

struct COMInitializer
{
COMInitializer(...)
{
CoInitialize(...);
}

~COMInitializer()
{
CoUninitialize();
}
};

some function or method
{
COMInitializer comInit;

...
}


Alex


 | 
Pages: 1
Prev: boolean
Next: asynchronous and synchronous call between apartments