Re: trouble with STL list initialization inside nested structure

From:

ytrembla@nyx.nyx.net (Yannick Tremblay)

Newsgroups:

comp.lang.c++

Date:

30 Sep 2009 09:46:07 GMT

Message-ID:

<1254303967.853973@irys.nyx.net>

In article <a64d6462-41f2-49ad-9925-5915d53ec564@t2g2000yqn.googlegroups.com>,
M A <medha.atre@gmail.com> wrote:

Thanks for the explanation Joshua. I wasn't aware of intricate
differences between "malloc" and "new"!
I just made a quick fix as follows:
------------------------------------

typedef struct pattern {
    int nodenum; // unique in the graph
    MyStruct ms;
} TP;

int main(int args, char **argv)
{

    TP *tp = new TP;

    struct row r1 = {1, (char *)"xyz"};

    tp->ms.bm.push_back(r1);

    list<struct row>::iterator itr = tp->ms.bm.begin();

    cout << (*itr).rowid << " " << (*itr).data << endl;
}
-------------------------------------
And it is working now.
Thanks a lot for your help. :)

Correction: you think it is working and it has not segfaulted on you
yet because in your test code you are using a static C string...

Let's look at it again:

struct row {
       int rowid;
       char *data;
       bool operator<(const struct row &a) const {
               return (this->rowid < a.rowid);
       };
};

OK, C++ containers use value semantic and copy data around. So what
will happen when you put this in a container is that the row will be
shallow copied element by element:

row r1 = { 1, (char *)"xyz" };
row r2 = r1;

now r1.data points to the "xyz" static string and r2.data point to the exact
same memory address. So if you modify the content of r1.data, you are
also modifying r2.

Things will become much worse if instead of using a static string
"xyz", you use some dynamic buffer inadvertently. Note that
dynamically allocating your char *data will not solve your problem,
only make it more complicated and create heap violation rather than
stack violation.

E.g.:

int main()
{
  char *buffer = strdup("Oh dear!");
  int i = 84;
  TP tp;
  row r1 = { i, buffer };
  tp.ms.bm.push_back(r1);

  // ...
  free(buffer);
  // ...
  list<row>::iterator itr = tp->ms.bm.begin();
  printf("%s\n", itr->data); // !!! Oh dear!
}

The simplest fix is:

struct row
{
  int i;
  std::string data;
  bool operator<(const row &a) const {
      return (this->rowid < a.rowid);
  };
}

The alternative is to implement it all yourself with a constructor, a
copy constructor and an assignement operator that allocate a buffer
for data and copy the input as well as a destructor that release the
memory. But that's a waste of effort.

typedef struct {
list<struct row> bm;
vector<struct row> vbm;
} MyStruct;

C+, remove the unecessary typedef and struct:
struct MyStruct {
list<row> bm;
vector<row> vbm;
}

typedef struct pattern {
int nodenum; // unique in the graph
MyStruct ms;
} TP;

Again reomve unecessary typedef

int main(int args, char **argv)
{
TP *tp = new TP;

Why dynamic allocation?
TP tp;
Works perfectly and should be the default. i.e. use new only when you must.

struct row r1 = {1, (char *)"xyz"};

Unecessary struct
You are assigning r1.data to a static string. (unless you made the
change to use a std::string or created a contructor the strcpy the
input). This is likely to cause you problems as your program grow. I
don't think you are planning to only ever assing static C string to
rows, are you?

tp->ms.bm.push_back(r1);
}

Hope this helps. Unfortunately, moving from C to C++ means a lot of
unleanring to do.

Yannick