vector of classes cloning problem

[realm]

I need to clone a vector of classes I have it so that it successfully clones but there is two errors: 1) upon exit it causes a crash which i take it is from an object already being deleted & 2) if I add a new object to the newely cloned vector it overwrites the first and last position of the vector.

This is what I have in my program.

I have 2 classes :

1- cnamevaluepair (which holds two string values)
2- cnamevaluepairlist( which has a vector of cnamevaluepair)

the vector def is showed in this typedef

typedef vector<CNameValuePair*> NVPLIST;
typedef vector<CNameValuePair*>::iterator NVPLISTITER;

I need to have the class clone itself from the main program with a call such as this:

new_list=old_list.clone();

So inside my cnamevaluepairlist.clone() I have this:

CNameValuePairList CNameValuePairList::clone()
{
  CNameValuePairList* temp2=new CNameValuePairList;
  string strValue;
  string strName;

  for (NVPLISTITER the_iterator=list.begin(); the_iterator != list.end(); the_iterator++)
  {
    strValue=(*the_iterator)->getValue();
    strName=(*the_iterator)->getName();
    temp2->add(strName,strValue);
  }
  return *temp2;
}

the list variable is of NVPLIST type. Inside my add(strName,strValue) function i have the following:

CNameValuePair* temp2=new CNameValue(strName,strValue);
list.push_back(temp2);

Do you know what I am doing wrong to cause the 2 errors stated earlier?

[PaulWendt]

Most clone()'s I see return a pointer to the newly created item
and don't return the item they're cloning by value. You may
experience a memory leak because you use new to create
the list and you're returning it by value ... meaning that it will
never get delete'd. Since you're returning it by value, then
either the copy constructor or the operator= will be called
[depending on the scenario]. I'd check that avenue first.
Assuming your clone() is modified to return a pointer, the logic
of what you're doing seems pretty straight-forward.

clone() is usually reserved for the case where you need a
"virtual constructor" of a class. It's not really germaine to the
conversation here, but unless you have a hierarchy of classes
and you need a virtual clone() method when you don't know
the type, then I'd just use a copy constructor and/or operator=.

--Paul

[realm]

I managed to fix the crashing problem although I still have the error of when I add a new value to the cloned list object it overwrites the last object in the vector and then adds the same one to the end. (on a side note if take out that call for adding a new object to the clone it crashes) I know it is a memory error somewhere, but my brain is burnt out from looking at this for so long. and I desperatly need help locating this problem. I have attached the code that I have. If someone could look at it and tell me where the error(s) is I would greatly appreciate it. on a side note, I cannot change the names/values returned for the functions. I can only add functions to the class(which I probably need to do, but I can't see what)

[PaulWendt]

Hello,

I looked at your code; there are a LOT of compile errors and
warnings on my compiler [Visual Studio .NET]. I recommend that
you double-check all functions' return values. Many of your bool
functions return nothing. When you turn the program in, just
make sure that you have gotten rid of all of the errors and
warnings; you'll thank yourself.

I modified your program to do what I recommended earlier and
everything worked. Here is the output [and there were no
crashes]:

size=0
EQUALS
size=3

FINISHED! IT NOW CONTAINS :
Mark=100
TESTOR=50
Dad=44

CLONE CONTAINS :
Mark=100
TESTOR=50
Dad=44
CLONE=5000

What did I do? I implemented the copy constructor and
operator=. The way clone() is defined in the header file, it needs
to return a value. Oddly enough, this is [almost] exactly what
operator= returns. The copy constructor is commonly
implemented in terms of operator= ... so really, the only tricky
thing you have to do is implement operator=. To get you started,
though, here is MY version of clone():

CNameValuePairListVector CNameValuePairListVector::clone()
    {
      CNameValuePairListVector newvec = *this;
      return newvec;
    }

newvec is NOT created with new here; it's just a local value.
There's no real need for new here because clone() returns an
object by value. Note that the first line of the function uses the
copy constructor. In my code, I have my copy constructor use
operator= to get its work done. For the record, my operator=
looks strikingly similar to your original version of clone() [minus the
syntax errors, of course].

I'm sorry I can't give you any more, but I don't want to do the
whole thing for you

--Paul
[/code]

[realm]

I got it working thanks for the push in the right direction. Although my glad tidings were somewhat dimmed upon the message of the teacher in which he says we shouldn't use a copy constructor or the operator=. oh well I'll keep this cuz it works and churn away on the other way if I have time. *sigh* But thanks alot Paul!

[mamio]

Hi,

- clone is usually implemented as returning a pointer to the cloned object. Is it correct to have it return a reference instead?

A related question is:

- If X is a reference to an object. Is it correct to write

delete &X? I mean, is &X technically a pointer?

[Graham]

Originally posted by mamio
Hi,

- clone is usually implemented as returning a pointer to the cloned object. Is it correct to have it return a reference instead?

A related question is:

- If X is a reference to an object. Is it correct to write

delete &X? I mean, is &X technically a pointer?

Syntactically, it's fine. Semantically, you could be in big trouble:

int i;
int * j = new int;
int &ri = i;
int &rj = *j;

delete &rj;  // OK (I'm fairly sure...)
delete &ri;  // Oops!

[mamio]

Thanks Graham,
now I see where the problem is,
but it is no worse than


int i;
int * pi = &i;

delete pi; // Oops!


is it?

M