🚀 go-pugleaf

"ScOe" <cbinders@hotmail.com> writes:

"Andrue Cope [TeamB]" <no.spam@spammy.com> writes:

> Questions on using VCL components should be asked in
> .VCL.COMPONENTS.USING. Follow-up set.

Agreed.

But there is a big C++ problem with the posted code, which is best dealt with
in .language.cpp.


> void __fastcall TCBZIconObject::SetImage (TGraphic *Value)
> {
> try {
>     FImage = new Graphics::TBitmap ();
>     FImage->Assign (Value);
>     }
> catch (...) {
>     try {
>         delete FImage;
>         FImage = new Graphics::TIcon ();
>         FImage->Assign (Value);
>         }
>     catch (...) {
>         }
>     }
> }
>
> is it ok to do it like this ?

No.

What you seem to want to do are the following operations:

1) allocate dynamic memory for a Graphics::TBitmap object
2) construct a Graphics::TBitmap object into that memory using
   Graphics::TBitmap's default constructor
3) assign the address of a TGraphic object to that Graphics::TBitmap object
   using the Assign() member function

Some of these operations may fail (I know for sure that the first one can, and
that's sufficient for my point); failure is signaled by an exception.


The first problem I see is that the exception handler part of SetImage()
attempts to redo the things that just failed. You'll have to work very hard
to convince me that this is the right thing to do. Why on earth should these
operations succeed after they they have just failed?


Then to the FImage data member. It seems to be used to sometimes own a
dynamically created Graphics::TBitmap object and sometimes not. From the
code you post, it further seems that FImage is of type Graphics::TBitmap *.

However it is very rarely a good idea to express the ownership of a
dynamically allocated resource through a "raw" pointer. The simplest and
therefore usually best thing is to not use a pointer at all, but a "real"
object instead. If that is not possible (because of VCL or other
restrictions), use a smart pointer object (such as a std::auto_ptr).

One of the benefits of not using a raw pointer is that you don't need to
write exception handling code except when you are handling exceptions. Making
FImage a std::auto_ptr<Graphics::TBitmap> would simplify the SetImage()
member to something like

void __fastcall TCBZIconObject::SetImage(TGraphic *Value)
{
  FImage.reset(new Graphics::TBitmap);
  FImage->Assign(Value);
}

This would also let a possible exception escape to the caller, which is a
Good Thing (tm).


Finally, I can't imagine that SetImage() should behave the same way when FImage
already owns an object and when it doesn't. My version of SetImage already
deals correctly with this issue; reset() will delete the currently owned
object before "adopting" the new one. But still, I think you should either
initialize FImage to own an object in the constructor(s) or add a check at the
beginning of SetImage(), as in

void __fastcall TCBZIconObject::SetImage(TGraphic *Value)
{
  if (FImage.get()==0)
    FImage.reset(new Graphics::TBitmap);

  FImage->Assign(Value);
}