Solved

Copy Constructors and parameter passing...???

Posted on 2002-03-08
16
226 Views
Last Modified: 2008-01-09
Hi,
I have a section of code which is passing a class as a parameter to a function ,The parameter is passed into the function (by value) and I do whatever I need to do with it,this much is fine (In other words the data is definitely being passed in in tact and I can use it to perform the required task's), however when I go to pass back the class I appear to lose all of the data and the program crashes.I read that to pass a class "properly" you have to write a copy constructor so I did this(or at least I think I did).Even with this the data is lost when I return the class The relevant pieces of code are below.

/* Function Takes in two parameters one holding the original image and the other to store the greyscale
The parameters are of type CSTScreenBuffer which is a class which stores an image*/

CSTScreenBuffer CGreyScale::ConvertToGreyScale(CSTScreenBuffer original, CSTScreenBuffer greyScale)
{
int x=0,y=0;               // Co-ordinate system
int imgheight,imgwidth;          // image height and width
int Red=0,Green=0,Blue=0;     // Pixel colour values
int GScaleValue=0;          // GreyScale values
BGRColor pixcol;          // Used to get Pixel colours


imgwidth = original.GetWidth(); // Get the image width
imgheight = original.GetHeight();// Get the image height


//Step through the image getting and setting pixel values
for(y = 0;y < imgheight;y++){
     for(x = 0;x < imgwidth;x++){
     Red = 0;//Re-Initialise red
        Green = 0;//Re-Initialise Green
        Blue = 0;//Re-Initialise Blue

        //Get the Pixel information
     pixcol = original.GetPoint(x,y);  
          Red = pixcol.m_R;  // Extract the red
          Green = pixcol.m_G;// Extract the green
          Blue = pixcol.m_B; // Extract the blue

        // Convert pixel value to Grey Scale
        GScaleValue = (int)((0.299*Red)+(0.587*Green)+    (0.114*Blue));

// Generate the new color reference for the given pixel
        greyScale.SetPoint        (x,y,GScaleValue,GScaleValue,GScaleValue);
     }
}
return greyScale;
}

/* Creates an instance of the Class GreyScale and uses it to convert an image held in original to Grey Scale. The returned image is placed in GreyScale.It then Draws the image out onto the screen*/

void CBmploaderDlg::OnGreyScale()
{
CGreyScale GScale;
CClientDC dc(this);
CPoint point = CPoint(250,1);

GreyScale =  GScale.ConvertToGreyScale(Original,GreyScale,dc.m_hDC);

GreyScale.Draw(&dc,point);
Invalidate(FALSE);
}


/* The copy Constructor copys all variables in the original image to the copied image*/

CSTScreenBuffer::CSTScreenBuffer(const CSTScreenBuffer& buffer)
{
m_hBitmap = buffer.m_hBitmap;
m_hSaveBitmap = buffer.m_hSaveBitmap;
m_nCorrectedWidth = buffer.m_nCorrectedWidth;
m_nHeight = buffer.m_nHeight;
m_nWidth = buffer.m_nWidth;
m_pBuffer = buffer.m_pBuffer;
m_pDC = buffer.m_pDC;
}

I think I have made it fairly clear. If not please say so and I will attempt to clarify any problems you may have

Cheers...
Ber...
0
Comment
Question by:Ber
  • 7
  • 5
  • 2
  • +1
16 Comments
 
LVL 86

Expert Comment

by:jkr
ID: 6850271
It's not a copy ctor problem - it seems that 'm_pBuffer' is deleted by the original instance when the destructor is called. Is that true?
0
 
LVL 2

Author Comment

by:Ber
ID: 6850331
Thats a good point, I never thought of that. Any idea on how to avoid this..???
0
 
LVL 2

Author Comment

by:Ber
ID: 6850343
Thats a good point, I never thought of that. Any idea on how to avoid this..???
0
Networking for the Cloud Era

Join Microsoft and Riverbed for a discussion and demonstration of enhancements to SteelConnect:
-One-click orchestration and cloud connectivity in Azure environments
-Tight integration of SD-WAN and WAN optimization capabilities
-Scalability and resiliency equal to a data center

 
LVL 86

Expert Comment

by:jkr
ID: 6850365
>>Any idea on how to avoid this..???

Yup :o)

To avoid this and expensive memory movement, use a cctor like


    CMemPtrHolder () : _pd ( NULL) {}

    // We'll need a cctor that takes care about the allocated data...
    CMemPtrHolder ( const CMemPtrHolder& CMemPtrHolder) {

        _pd = CMemPtrHolder._takeover();

    }
    CMemPtrHolder ( _DATA* __pd, uint __size) {


        _pd = new _DATA [ __size];


    };

    ~CMemPtrHolder() {

        if ( _pd) {

            delete [] _pd;
        }
    }

    mutable _DATA*  _pd; // data

private:
    _DATA* _takeover () const {

        _DATA*  _pd_ret = _pd;

        _pd = NULL;

        return _pd_ret;
    }

};
0
 
LVL 2

Author Comment

by:Ber
ID: 6850447
I hate to ask but could you please give a little more explanation of this code. I have a fair idea(I think) of what its doing but It would help...??

Cheers...
Ber...
0
 
LVL 86

Expert Comment

by:jkr
ID: 6850474
Well, the idea is to set the data pointer to NULL - that will let the object you're copying from 'know' that it shouldn't delete the memory (you could also set a flag). The problem now is that a cctor deals with a const reference, thus requiring that the data pointer is declared as "mutable", making it possible to change that value even if the object has a "const" attribute.
0
 

Expert Comment

by:boneTKE
ID: 6851082
You have to be careful about writing copy ctors when you class "manages remote memory" .

For example, if pointers within you class point to memory dynamically allocated by your class, then you must allocate an equivalent chunk of memory in your copy constructor, and then copy the data.  If you don't, you end up with pointers in two different objects pointing to the same memory (and therefore the same data).  This is sometimes called a "shallow" copy.  Destruction of either object could invalidate the other pointer.

It's not clear what you're doing with your pointers, but this could be your problem.

Good luck.
0
 

Expert Comment

by:boneTKE
ID: 6851084
You have to be careful about writing copy ctors when you class "manages remote memory" .

For example, if pointers within you class point to memory dynamically allocated by your class, then you must allocate an equivalent chunk of memory in your copy constructor, and then copy the data.  If you don't, you end up with pointers in two different objects pointing to the same memory (and therefore the same data).  This is sometimes called a "shallow" copy.  Destruction of either object could invalidate the other pointer.

It's not clear what you're doing with your pointers, but this could be your problem.

Good luck.
0
 
LVL 2

Author Comment

by:Ber
ID: 6853473
jkr I have tried to implement your code but I just do not understand it well enough to get it to work. Another thing I have noticed is that the constructor is also copying things like the HBITMAP and the HDC which the destructor is also deleting.Any ideas of how to generate a new handle to the same object.

boneTKE Thats exactly whats happening but how do I avoid it as stated above its not just copying pointers to memory its also copying the handles  to the bitmap and device context

I have made a temporary fix by throwing it into a list but I'd rather get the constructor working properly

Cheers...
Ber...
0
 
LVL 86

Expert Comment

by:jkr
ID: 6853494
>>Any ideas of how to generate a new handle to the same
>>object.

Well, the only reasonable way is to not destroy them. With a little modification to the above example to make it more "general", you should get a better idea on how to do that:

class CResourceHolder {

    CResourceHolder () : m_bDeleteResources ( TRUE);

   // We'll need a cctor that takes care about the allocated data...
   CResourceHolder ( const CResourceHolder& r) {

       r._takeover();

       // copy members, except the m_bDeleteResources flag

   }


   ~CResourceHolder() {

       if ( m_bDeleteResources) {

           // delete resources
       }
   }

   mutable BOOL  m_bDeleteResources; // data

private:

   // set a flag to indicate that the dtor should not delete the resources
   void _takeover () const {

     m_bDeleteResources = FALSE;

   }

};


0
 
LVL 2

Author Comment

by:Ber
ID: 6853614
jkr I have it implemented correctedly..I Think

The only thing I can't see is where the size comes from..??

>> CMemPtrHolder ( _DATA* __pd, >> uint __size << ) {
>>     _pd = new _DATA [ __size];
>> };

do I have to pass it in or can I get it from somewhere else..??

Furthermore I am not that familiar with constructors and was wondering how the second constructor is called. In other words do I have to call it or is it called automatically..??

Cheers..
Ber..
0
 
LVL 86

Accepted Solution

by:
jkr earned 300 total points
ID: 6853703
Well, the size describes the size of the memory buffer - but that was only an example, you should better try to adapt your own code...
0
 
LVL 49

Expert Comment

by:DanRollins
ID: 6854282
I think there are other significant problems.  This is the only fn named ConvertToGreyScale

CSTScreenBuffer CGreyScale::ConvertToGreyScale(CSTScreenBuffer original, CSTScreenBuffer greyScale)

and later, you use:

        void CBmploaderDlg::OnGreyScale()
        {
        CGreyScale GScale;
        CClientDC dc(this);
        CPoint point = CPoint(250,1);

        GreyScale =  GScale.ConvertToGreyScale(Original,GreyScale,dc.m_hDC);

        GreyScale.Draw(&dc,point);
        Invalidate(FALSE);
}

Which calls a three-parameter version of that function.  Please post that function.

Also, the above code uses some variables named GreyScale and Original which are not declared locally and are not  parameters to the call.  Are these members of class CBmploaderDlg?  If so, you will find that prefixing their names with 'm_' will help you avoid some confusion.  Are they global variables?

Ther is no reason to add a complex memroy allocation system to this.  Just avoid the automatic copying and destruction inherent in y9our use of passing structures by value.  Just pass pointers:

void CGreyScale::Convert(CSTScreenBuffer* pSrc, CSTScreenBuffer* pDest)
     BGRColor pixcol;           // Used to get Pixel colours

     int nImgWidth= pSrc->GetWidth(); // Get the image width
     int nImgHeight= pSrc->GetHeight();// Get the image height

     for(int y= 0; y < nImgHeight; y++ ){
          for(int x= 0; x < nImgWidth; x++ ){
               pixcol= pSrc->GetPoint(x,y);  
               int nRed=   pixcol.m_R;  // Extract the red
               int nGreen= pixcol.m_G;  // Extract the green
               int nBlue=  pixcol.m_B;  // Extract the blue

               // Convert pixel value to Grey Scale
               int nGrey= (int)( (0.299*nRed)+(0.587*nGreen)+ (0.114*nBlue) );
               pDest->SetPoint( x,y, nGrey,nGrey,nGrey );
          }
     }
}

That way, the caller can control the creation and destruction of the data structures.   For instance, a typical call might be:

CSTScreenBuffer cBufSrc( 100,100 );
CSTScreenBuffer cBufDst( 100,100 );
...
cGreyScale.Convert( &cBufSrc, &cBufDest ) ;

..etc...

-- Dan
0
 
LVL 2

Author Comment

by:Ber
ID: 6854825
Dan
The Line:
>> GreyScale =  GScale.ConvertToGreyScale(Original,GreyScale,dc.m_hDC);

has an error, I was modifying the code at the time and forgot to remove it when I was putting the code on the web, The dc.m_hDC was just being passed So I could Check something, it is now removed. As for passing pointers this is what I am now doing but I still had the same problem because I still had to edit the image I was working with which meant that I was changing pixel values which then affected the next set of calculations (Note: Actually It does not affect the GreyScale function but it does have a major impact on filtration and Edge detection).I have got around this problem by creating a copy of the m_pBuffer in memory and doing the calculations on that, while updating the original with the results, this works ok for now but I may come back and try to change it later..


Cheers...
Ber...
0
 
LVL 49

Expert Comment

by:DanRollins
ID: 6855149
Whatever your final mechanism, I think that you will be best served by not passing and returning structures by value.  The compiler can end up making temporary copies of entire structures.  Aside from being terribly ineffcient, this opens the doors to many hard-to-locate bugs.

-- Dan
0
 
LVL 2

Author Comment

by:Ber
ID: 6856057
Jkr
     Thank you for your help...

Everyone Else thank you for your help and pointers,It is greatly appreciated

Cheers...
Ber...
0

Featured Post

Free Tool: Subnet Calculator

The subnet calculator helps you design networks by taking an IP address and network mask and returning information such as network, broadcast address, and host range.

One of a set of tools we're offering as a way of saying thank you for being a part of the community.

Question has a verified solution.

If you are experiencing a similar issue, please ask a related question

Suggested Solutions

Introduction This article is the first in a series of articles about the C/C++ Visual Studio Express debugger.  It provides a quick start guide in using the debugger. Part 2 focuses on additional topics in breakpoints.  Lastly, Part 3 focuses on th…
  Included as part of the C++ Standard Template Library (STL) is a collection of generic containers. Each of these containers serves a different purpose and has different pros and cons. It is often difficult to decide which container to use and …
The goal of the video will be to teach the user the concept of local variables and scope. An example of a locally defined variable will be given as well as an explanation of what scope is in C++. The local variable and concept of scope will be relat…
The viewer will learn how to pass data into a function in C++. This is one step further in using functions. Instead of only printing text onto the console, the function will be able to perform calculations with argumentents given by the user.

809 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question