0

I have a class that I want to contain multiple objects of something I created. Right now the code that works is:

process.h:

private:
  myObj *data;

process.cc:

data = new myObj[10];

I would like to pass a value to the constructor however, so I tried to convert it to a std::vector (after modifying constructor to take a value).

process.h:

private:
  std::vector<myObj> data;

process.cc:

for (int m=0; m<10; m++) data.push_back( myObj(1.2) );

When I try that it crashes upon execution with

*** glibc detected *** ... corrupted double-linked list: ... ***

And the backtrace in gdb shows an error in the destructor when I tried to free some memory for other arrays I allocated. A search didn't show up anything that was obvious. I am using a few static member variables in myObj, could that be an issue?

Eric C.
  • 376
  • 3
  • 16
  • 2
    Your program has an out-of-bounds memory reference somewhere else in the code. When the data moved from the inside of the object to somewhere on the heap it changed the symptoms of the problem. – Mark Ransom Jun 15 '12 at 22:09
  • 1
    This makes little sense. You talk about an array, but then a vector. Which is it? And the crash is at compilation? Really? – David Heffernan Jun 15 '12 at 22:10
  • 1
    @cooleric1234 Static members has nothing to do as far I can see. There is something else going on. Please post small snippet of code where you can reproduce the issue. – Mahesh Jun 15 '12 at 22:11
  • 1
    Can you show the skeleton of your class (at least the ctor and dtor)? – Eitan T Jun 15 '12 at 22:15
  • @DavidHeffernan Sorry, not at compilation, I don't know what I was thinking. Within `myObj` I am allocating a couple of arrays. But then I want a vector of myObj objects. Does that make sense? I guess it could be a problem with the arrays I'm allocating. It's complicated but I don't know if I can post my code or not, I'll see. I was just wondering if there was some rule I wasn't familiar with. – Eric C. Jun 15 '12 at 22:25
  • 1
    The code you've posted is fake anyway. The real code compiles. The code here doesn't. Hard to help if you post made up code. Not much fun trying to guess what your code really looks like. Post real code. – David Heffernan Jun 15 '12 at 22:30
  • Better yet, post a [short, self contained, compilable example](http://sscce.org/) which demonstrates the problem. – David Jun 15 '12 at 22:32
  • 3
    @cooleric1234, If you're allocating memory, you'd better be following the Rule of Three, especially when putting it in the vector, where it shallow copies the elements unless otherwise specified. In C++11, that changes a bit, but the idea is the same. – chris Jun 15 '12 at 22:48
  • possible duplicate of [What is The Rule of Three?](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) – fredoverflow Jun 16 '12 at 08:05

2 Answers2

3

You are experiencing a double deletion bug. Consider this simple example:

struct Other {};

struct MyObj {
    Other *p;

    MyObj () : p(new Other) {}
    ~MyObj () { delete p; }
};

std::vector<MyObj> data;

data.push_back(MyObj());

The temporary object that is pushed onto data is stored properly. However, since it is a temporary, it is destroyed right after the push. This means, the p member is deleted when the temporary is destroyed, so the vector's version of the copy has a dangling pointer. When the vector object is destroyed, the pointer will be deleted again, resulting in a corrupted heap. The error message you received was from the glibc code complaining about the resulting bad state.

To fix this problem, a proper copy constructor should be defined, to pass ownership of the objects from the temporary to the destination. The rule of three says we should define the assignment operator as well.

struct MyObj {
    mutable Other *p;

    MyObj () : p(new Other) {}
    MyObj (const MyObj &o) : p(o.p) { o.p = 0; }
    ~MyObj () { delete p; }
    const MyObj & operator = (MyObj o) {
        using namespace std;
        swap(*this, o);
        return *this;
    }
};

The use of mutable is required to be able to modify the p member when the instance is const, and const was needed because temporaries are being passed in to the copy constructor. With this change, pushing items into the vector work fine.

A better solution would be to define p to use a unique_ptr instead.

struct MyObj {
    std::unique_ptr<Other> p;

    MyObj () : p(new Other) {}
};

No destructor is needed in this example, since the default destructor will destruct p, which will cause the Other instance to be deleted by unique_ptr.

jxh
  • 69,070
  • 8
  • 110
  • 193
  • no, 99.9% of the time you want a std::unique_ptr instead of a std::shared_ptr – Mooing Duck Jun 16 '12 at 09:51
  • @MooingDuck: You're right. Old habits die hard. Edit has been made. Thanks and regards – jxh Jun 16 '12 at 14:07
  • Thanks for that, I don't have access to my code right now but this looks like it explains what's going on. I didn't realize it allocates a temporary object and then copies it over. I don't think I have a copy constructor as I didn't intend on doing any copying. – Eric C. Jun 16 '12 at 16:20
  • @MooingDuck: It's too bad there is no version of `weak_ptr` to use with `unique_ptr`. – jxh Jun 16 '12 at 16:39
  • usually the assignment operator uses the copy and swap for the exception guarantees. – Mooing Duck Jun 16 '12 at 21:44
  • @user315052: I don't think it's overkill. It works on all data types, in all situations, and is fewer lines of code, and is _almost_ as fast. – Mooing Duck Jun 16 '12 at 23:02
  • @user315052: corrected your copy&swap to be more typical, work more efficiently for moved types, and be able to use overloaded swap functions. – Mooing Duck Jun 16 '12 at 23:28
  • let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/12649/discussion-between-user315052-and-mooing-duck) – jxh Jun 16 '12 at 23:31
-1

You're trying to use a vector to store multiple objects within a class? I have run into this problem also and the only way I could fix this was to place the function you are using the vector in, in the header. What I believe is happening is you're providing the vector with a type, in this case, myObj, but the .cpp can not see what type you've defined the vector as. So sticking the function inside the Header seems to fix it. I believe there are other ways around this but I've not looked into the problem much.

example code:

class A
{
 private:
 vector<myObj> data;
 public:
 A();
 ~A();

 printData()
 {
     for(int i = 0; i < data.size(); i++)
     {
         printf("X position: %.2f Y position: %.2f Z position: %.2f \n", data.at(i).x, data.at(i).y, data.at(i).z);
     }
 };
}

This may be the problem, or, it is your naming convention. I am not sure what you are doing but how does data *myObj; and data = new myObj[10]; actually work? Wouldn't it be myObj = new data[10]? If so then your vector would be:

vector<data> myObj;
M Davies
  • 255
  • 1
  • 3
  • 11