Solved

How can I deep copy an stl list of pointers. In regards to copy constructor and operator= for a class containing an stl list?

Posted on 2004-04-22
21
800 Views
Last Modified: 2013-12-14
Hi experts,
I have a class containing a list:\
class ac{
 CString content;
  ......
}
class we {
  typedef list<ac*> ls;
  ls::iterator listPos;
  ls myList;
  .....
  we(const we &rValue) ;   //This is what I need ;-)
}
What I was trying is:
we::we(const we &rValue) {
  listPos = rValue.listPos;
  for(listPos= rValue.myList.begin(); listPos != rValue.myList.end(); listPos++)
     myList.push_back(*listPos);
}
I feel like this is somehow close to what I want but
--There is apparently no acceptable conversion from const listPos to listPos.
Is there a simpler approach to copying a list of Pointers //I mean really copying values not merely pointers.
The thing why I am doing this is because I am doing something like this in another place:
we *whatever = new we(some arguments);
So when the class falls out of scope everything is gone.
Any comment would be appreciated
Jens
0
Comment
Question by:allmer
  • 8
  • 8
  • 3
  • +2
21 Comments
 
LVL 86

Expert Comment

by:jkr
ID: 10889541
>>I feel like this is somehow close to what I want

I feel like

we::we(const we &rValue) {
 listPos = rValue.listPos;
 myList = rValue.mylist;
}

is closer...


0
 
LVL 8

Expert Comment

by:_corey_
ID: 10889615
Well, one thing is that assignment:
listPos = rValue.listPos;

is not needed, and you should re-create the iterator the way you do in the for statement.
--

Also, are you sure that you need a pointer list?  Could you do this with a value based list and use the default copy constructor/constructor?
0
 
LVL 5

Author Comment

by:allmer
ID: 10890110
@jkr
I was actually using the same approach as you before
but when I do this
we *whatever = new we(some arguments);
whatever does not contain the list which was created in we(some arguments).
Seems to me that only the pointers are copied and they are of course deleted when
we(some arguments) falls out of scope ;-)
~we() {
  for(...)
    delete(*listPos);  
  myList.clear();
}
If I forget about deleting in the destructor, which is of course not the right thing to do, I should say.
whatever contains the elements formerly contained in we(some arguments).

@corey
I probably wouldn't need a pointer list for this combination of classes but that's not what
I work with ;-)

If I try this I get a compiler error:
we *whatever(some arguments);
so for me it seems I cannot avoid the new operation here
but that also means that I need a working operator= function and a copy contructor.
Plus I cannot delete what I haven't newed so the destructor wouldn't work.

Regards,
Jens

0
 
LVL 5

Author Comment

by:allmer
ID: 10890257
If I omit the line:
listPos = rValue.listPos;
the same error comes up in the next line:
for(listPos= rValue.myList.begin(); listPos != rValue.myList.end(); listPos++)
This is quite weird.
It would probably work if I do not add const to my operator= and copy constructor argument lists.
Wouldn't be the best workaround I guess.
0
 
LVL 39

Expert Comment

by:itsmeandnobodyelse
ID: 10890620

>> I mean really copying values not merely pointers.

Try this:

we::we(const we &rValue) {
  listPos = rValue.listPos;
  for(listPos= rValue.myList.begin(); listPos != rValue.myList.end(); listPos++)
     myList.push_back(new ac(*(*listPos));
}

That will work if class ac has a copy constructor.

BTW, 'we' and 'ac' are poor class names.

0
 
LVL 8

Expert Comment

by:_corey_
ID: 10890661
I'm still not sure of the point of listPos = rValue.listPos;

Could you explain what you're doing there?
0
 
LVL 39

Expert Comment

by:itsmeandnobodyelse
ID: 10890754
>> myList.push_back(new ac(*(*listPos));

There is one right parenthesis missing, should be  

   myList.push_back(new ac(*(*listPos)));

>> listPos = rValue.listPos;
>> is not needed

That means you don't need an iterator as a member. Do this:

we::we(const we &rValue) {
  ls::iterator iter;
  for(iter= rValue.myList.begin(); iter != rValue.myList.end(); iter++)
     myList.push_back(new ac(*(*iter)));
}

and get rid of that member listPos.

Regards, Alex
0
 
LVL 8

Expert Comment

by:_corey_
ID: 10891120
Well, if I'm not mistaken, the iterator is invalid if you delete/etc a member in the list, so storing it constantly isn't that useful.
0
 
LVL 39

Expert Comment

by:itsmeandnobodyelse
ID: 10891245
>> the iterator is invalid if you delete/etc a member in the list

That is not a problem, as with

     listPos = rValue.myList.begin()

it gets a valid entry again.

But as the iterator gets invalid at end of any loop it makes no sense to store it as a member. Furthermore, as an STL iterator isn't associated with a specific instance of the container class, you could use your own member listPos and didn't need using rValue.listPos as iterator.

Regards, Alex


0
 
LVL 5

Author Comment

by:allmer
ID: 10892485
This all sounds too good.
I get a compiler error, however:
error C2679: binary '=' : no operator found which takes a right-hand operand of type 'std::list<_Ty>::const_iterator' (or there is no acceptable conversion)
        with
        [
            _Ty=CCutScheme *
        ]
which points me to this line:

CProtease CProtease::operator=(const CProtease & rValue)
{
      csl::iterator i;
//produces error see above in the for loop
      for(i = rValue.consensusSequenceList.begin(); i != rValue.consensusSequenceList.end(); i++)
            consensusSequenceList.push_back(new CCutScheme(*(*i)));
      name = rValue.name;
      return(*this);
}
Same error for the copy constructor.
@itsmeandnobodyelse
do you like this class names?
@corey
You are right about the iterator but it is not the iterator I want
because I learned how to get, pass and create one here:
http://www.experts-exchange.com/Programming/Programming_Languages/Cplusplus/Q_20962117.html
Nicely done by khkremer, I have to add.
I want all the data members of the linked list copied to the class not just referenced.
Cheers
Jens

0
How to run any project with ease

Manage projects of all sizes how you want. Great for personal to-do lists, project milestones, team priorities and launch plans.
- Combine task lists, docs, spreadsheets, and chat in one
- View and edit from mobile/offline
- Cut down on emails

 
LVL 39

Expert Comment

by:itsmeandnobodyelse
ID: 10892634
>> csl::iterator i;

What is csl?? Can you post the typedef to that??

There should be a typedef in class declaration of CProtease, similar to that

   class CProtease
   {
           ...
           typedef std::list<_Ty>::const_iterator csl;
    };

And you shouldn't use a  'csl' in another class. And CProtease::consensusSequenceList should be of type std::list<_Ty> . (Don't like that classname _Ty either).

Maybe you have to use std::list<_Ty>::iterator and not std::list<_Ty>::const_iterator ???

Regards, Alex
 
0
 
LVL 5

Author Comment

by:allmer
ID: 10892724
Sure Alex,
that's why I tried it with the easy example I made up at the beginning ;-)
Anyway here is the further information.
csl is unique to CProtein and is typedeffed below:

class CProtein {
public:                //nobody else is ever gonna use these classes make em public for now
typedef list<CCutScheme*> csl;
      csl consensusSequenceList;
      csl::iterator listPos;
//some other stuff
};

Greetz
Jens
0
 
LVL 5

Author Comment

by:allmer
ID: 10892744
BTW
CCutScheme has both, a functioning operator= function
and a working copy constructor.
Doesn't contain lists or any other pointer structures so that was easy.
Even I got that straight ;-)
0
 
LVL 39

Expert Comment

by:itsmeandnobodyelse
ID: 10894383
class CProtein and class CProtease both are good classnames ( i like them ). However, these class names are different and if you had a typedef within the class scope of CProtein you can't use that typedef in a member function of class CProtease.  And what is CCutScheme? Is that equivalent to _Ty? I don't know exactly why you are messing up your class relationships by 'typedefing' all good class names using redundant abbreviations.

You should post all your class declarations to give us a chance not only guessing what's going wrong but to know for sure.

Regards, Alex

0
 
LVL 5

Expert Comment

by:rendaduiyan
ID: 10895508
The best way is to use swap member of list.
myList.swap(rValue.myList)
0
 
LVL 5

Author Comment

by:allmer
ID: 10900891
I guess you are right Alex.
The problem seems to be somewhere else.
As far as I can see, my copy constructors or operator= functions are never called.
So there may be something wrong with that.
See the headers for the two classes below.
If you need anything beside the headers please post.
I will increase points because this may take longer than I thought it would.
Seems to be a bigger problem.


@rendaduiyan
looks good but fails to copy the const list rValue.
Seems like a const list cannot be iterated through ... strange since no attempt to change the data is made.

OK, I just now did some serious debugging // at least I tried so I came up with this problem:
class CSequenceSuggestions contains a class variable:
COptions options. //this is the only definition in the header of CSequencSuggestions
.
If I set a break point in the constructor:
COptions::COptions(void)
{
      PRECISION = 700;
      PRECISION /= 1000000;
      FRUNMINID = 5;
      SRUNMINID = 3;
      MININTRONLENGTH = 1;
      MAXINTRONLENGTH = 700;
      PATHTOCLEAVINGENZYMES = "c:\\gpfc++\\enzymes.cdf";
      CCuttingEnzyme cutter;
      if(cutter.ReadEnzymesFromFile(PATHTOCLEAVINGENZYMES))
            PROTEASE = cutter.GetEnzyme("TRYPSIN");
      else {
            PROTEASE->name = "TRYPSIN";
            PROTEASE->consensusSequenceList.push_back(new CCutScheme(0,"R"));
            PROTEASE->consensusSequenceList.push_back(new CCutScheme(0,"K"));
      }
}
CProtease is well defined at this point ;-)
Next time I call options every variable therein is fine.
Except for Protease which then is undefined.
Header for COptions:
CCuttingEnzyme contains a list of CProtease which contains a list of CCutScheme.
As I stated before this used to work for me until I learned how to actually delete list members.
When I implemented this in all the destructors above this happend.
So I thought it was somehow related.
Please tell me if you need to know anything else.
Thank you very much so far
Jens

#pragma once
#include "protease.h"

class COptions
{
public:
      COptions(void);
      ~COptions(void);
      int                  FRUNMINID,
                        SRUNMINID,
                        SRUNABSMIN,
                        MININTRONLENGTH,
                        MAXINTRONLENGTH;
      double            PRECISION;
      CProtease      *PROTEASE;
      CString            PATHTOCLEAVINGENZYMES;
};
0
 
LVL 39

Expert Comment

by:itsmeandnobodyelse
ID: 10901087
>> CProtease is well defined at this point ;-)

PROTEASE is a pointer in COptions. However you never assign storage to it in the constructor.

So,  
 
    PROTEASE->name = "TRYPSIN";

will definitively fail if the else branch wins. You have to do this somewhere in Constructor:

    PROTEASE = new CProtease(...);

>> Next time I call options every variable therein is fine

A constructor runs only once for any instance. A 'next' call constructs a new COptions instance that knows nothing from a previous call.

>> Except for Protease which then is undefined.

I think, first time cutter.ReadEnzymesFromFile(PATHTOCLEAVINGENZYMES)) is true and you get a valid PROTEASE pointer from there, second time it is false and code crashes as the pointer is invalid.

Regards, Alex




0
 
LVL 39

Expert Comment

by:itsmeandnobodyelse
ID: 10901155
You shouldn't use pointer members as it isn't necessary. Change PROTEASE from CProtease* to CProtease.

class COptions
{
public:
     COptions(void);
     ~COptions(void);
     int               FRUNMINID,
                    SRUNMINID,
                    SRUNABSMIN,
                    MININTRONLENGTH,
                    MAXINTRONLENGTH;
     double          PRECISION;
     CProtease     PROTEASE;
     CString         PATHTOCLEAVINGENZYMES;
};

Then, you have to change in constructor

        PROTEASE = cutter.GetEnzyme("TRYPSIN");

to

        PROTEASE = *cutter.GetEnzyme("TRYPSIN");      // Or change type of GetEnzyme(..) to CProtease also.

Now the assign operator gets called.

Further changes are:

          PROTEASE->name = "TRYPSIN";
          PROTEASE->consensusSequenceList.push_back(new CCutScheme(0,"R"));
          PROTEASE->consensusSequenceList.push_back(new CCutScheme(0,"K"));
to

          PROTEASE.name = "TRYPSIN";
          PROTEASE.consensusSequenceList.push_back(new CCutScheme(0,"R"));
          PROTEASE.consensusSequenceList.push_back(new CCutScheme(0,"K"));

That applies for all occurences of PROTEASE in any of your member functions.

Regards, Alex





0
 
LVL 5

Author Comment

by:allmer
ID: 10901162
OK I fixed it!
PROTEASE = cutter.GetEnzyme("TRYPSIN");
Fix->  PROTEASE = new CProtease(*cutter.GetEnzyme("TRYPSIN"));
I just have a pointer here so as soon as cutter falls out of scope and deletes
its list the pointer is invalid. So when I implemented the actual deletion of the
list members I got this problem here.

It all breaks down to copying the list now.
Means we are back at the original question.
How can I have the list actually copied.

What bugs me is that with a standard declaration of the cc and op= functions it will not compile:
CProtease::CProtease(const CProtease &rValue) {
this declaration will not let me access the list in rValue:
      consensusSequenceList.swap(rValue.consensusSequenceList);   //this doesn't work
      csl::iterator i;
      for(i = rValue.consensusSequenceList.begin(); i != rValue.consensusSequenceList.end(); i++)
//and this doesn't work either.
            consensusSequenceList.push_back(new CCutScheme(*(*i)));
      name = rValue.name;
}
With this declaration it works fine:
CProtease::CProtease(CProtease &rValue) {
}
But I would like it to be standard (const).
Thanks, Jens
0
 
LVL 39

Accepted Solution

by:
itsmeandnobodyelse earned 150 total points
ID: 10901385
Ok because of const  CProtease &rValue you may not write to any member of rValue and may not use any member function on rValue that isn't const.

1. why using swap()?

swap() isn't a const function i suppose because the name implies you want to replace list of this object with list of rValue. So it fails. But why swapping at all?

2.  rValue.consensusSequenceList.begin();  fails

That is because an iterator on a list isn't const either.

3. What to do?

Try this:

CProtease::CProtease(const CProtease &rValue) {
     consensusSequenceList = rValue.consensusSequenceList;
     csl::iterator i;
     for(i = consensusSequenceList.begin(); i != consensusSequenceList.end(); i++)
         *i = new CCutScheme(*(*i));
     name = rValue.name;
}

>> *i = new CCutScheme(*(*i));

That looks weird, but it makes a 'copy' of the object whose pointer had copied flat when assigning the list _AND_ assigns the pointer of the copy to the list entry.

That all would be much easier if you hadn't used pointers as template arguments but objects. Then

     consensusSequenceList = rValue.consensusSequenceList;

had done all the job.

Regards, Alex

0
 
LVL 5

Author Comment

by:allmer
ID: 10902613
Great Alex,
how do you come up with that stuff?
I am sure the pointers I push onto the list at this point may as well be
objects but I would like to have a deeper understanding of the matter.
Besides I will need to handle bigger objects soon so it will be of use to
know how to deal with pointer lists.

I like this: "That looks weird, but it makes a 'copy' of the object whose pointer had copied flat when assigning the list _AND_ assigns the pointer of the copy to the list entry.".

I will assign you all the points if there aren't any objections to that.

I know all of you did a great job and invested a lot of time.
I really apreciate that.

BTW is anyone from Philly, so I could hire you to sit together with me an look over all my code for a weekend or so?
I guess, I really need some real time help.
0

Featured Post

Highfive Gives IT Their Time Back

Highfive is so simple that setting up every meeting room takes just minutes and every employee will be able to start or join a call from any room with ease. Never be called into a meeting just to get it started again. This is how video conferencing should work!

Join & Write a Comment

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…
Jaspersoft Studio is a plugin for Eclipse that lets you create reports from a datasource.  In this article, we'll go over creating a report from a default template and setting up a datasource that connects to your database.
The viewer will learn how to use NetBeans IDE 8.0 for Windows to connect to a MySQL database. Open Services Panel: Create a new connection using New Connection Wizard: Create a test database called eetutorial: Create a new test tabel called ee…
The goal of the video will be to teach the user the difference and consequence of passing data by value vs passing data by reference in C++. An example of passing data by value as well as an example of passing data by reference will be be given. Bot…

743 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

Need Help in Real-Time?

Connect with top rated Experts

11 Experts available now in Live!

Get 1:1 Help Now