Declaring vector of pointer as private class member

[activecat]

Below is a working program.
Please note that the vector <int*> pointer is declared as a public member of the class A.

The problem arises when I try to make it a private (instead of public).
First, let's look at the File 1 below. It compiles and works fine.

File 1: main.cpp (working fine)

#include <vector>
#include <iostream>
using namespace std;


class A
{
    public:
        vector <int*> pointer;      // shall be private !
};

A *a [3];


void f1()
{
    for (int i=0; i < 3; i++)
    {
        cout << "Iteration " << i+1 << ": " << endl;

        for ( vector<int*>::iterator j = a[i]->pointer.begin(); 
                                    j != a[i]->pointer.end();
                                    ++j )
            cout << " Retrieved Data = " << **j <<  endl;

        cout << endl;
    }
}


int main()
{
    for (int i=0; i < 3; i++)
      a[i] = new A;


    for (int i=0; i < 3; i++)
    {
        cout << "Iteration " << i+1 << ":" << endl;
        for (int j=0; j < 5; j++)
        {
            int value =j + i * 10 + 1;
            cout << " Storing Data = " << value << endl;

            int *x = new int;
            *x = value;
            a[i]->pointer.push_back( x );   // vector

        }
        cout << endl;
    }

    cout << endl;
    f1();

    return 0;
}

It compiles nicely and produce following output.

Output of main.cpp of file1:

Iteration 1:
 Storing Data = 1
 Storing Data = 2
 Storing Data = 3
 Storing Data = 4
 Storing Data = 5

Iteration 2:
 Storing Data = 11
 Storing Data = 12
 Storing Data = 13
 Storing Data = 14
 Storing Data = 15

Iteration 3:
 Storing Data = 21
 Storing Data = 22
 Storing Data = 23
 Storing Data = 24
 Storing Data = 25


Iteration 1: 
 Retrieved Data = 1
 Retrieved Data = 2
 Retrieved Data = 3
 Retrieved Data = 4
 Retrieved Data = 5

Iteration 2: 
 Retrieved Data = 11
 Retrieved Data = 12
 Retrieved Data = 13
 Retrieved Data = 14
 Retrieved Data = 15

Iteration 3: 
 Retrieved Data = 21
 Retrieved Data = 22
 Retrieved Data = 23
 Retrieved Data = 24
 Retrieved Data = 25

Then the vector <int*> pointer is declared as private, as below:

File 2: main.cpp (problematic code)

#include <vector>
#include <iostream>
using namespace std;


class A
{
    private:
        vector <int*> pointer;         // it is private now !

    public:
        vector <int*> getPointer()   { return pointer; }
};

A *a [3];


void f1()
{
    for (int i=0; i < 3; i++)
    {
        cout << "Iteration " << i+1 << ": " << endl;

        for ( vector<int*>::iterator j = a[i]->getPointer().begin();
                                    j != a[i]->getPointer().end(); 
                                    j++  )
            cout << " Retrieved Data = " << **j <<  endl;

        cout << endl;
    }
}


int main()
{
    for (int i=0; i < 3; i++)
      a[i] = new A;


    for (int i=0; i < 3; i++)
    {
        cout << "Iteration " << i+1 << ":" << endl;
        for (int j=0; j < 5; j++)
        {
            int value =j + i * 10 + 1;
            cout << " Storing Data = " << value << endl;

            int *x = new int;
            *x = value;

            a[i]->getPointer().push_back(x);

        }
        cout << endl;
    }

    cout << endl;
    f1();

    return 0;
}

The output becomes incorrect, as follows.

Output of main.cpp of file2:

Iteration 1:
 Storing Data = 1
 Storing Data = 2
 Storing Data = 3
 Storing Data = 4
 Storing Data = 5

Iteration 2:
 Storing Data = 11
 Storing Data = 12
 Storing Data = 13
 Storing Data = 14
 Storing Data = 15

Iteration 3:
 Storing Data = 21
 Storing Data = 22
 Storing Data = 23
 Storing Data = 24
 Storing Data = 25


Iteration 1: 

Iteration 2: 

Iteration 3: 

So it fails to retrieve Data correctly!
How to solve..?

Thanks in advance.

[chhquan]

a[i]->getPointer().push_back(x);
a[i]->getPointer() returns a new copy of vector in a[i]. You have just push value in it, but not the vector in a[i].

[activecat]

Thanks. Solution found !
The key is to define the getPointer() as vector <int*> & .


Working code 1: (Option A)

#include <vector>
#include <iostream>
using namespace std;


class A
{
    private:
        vector <int*> pointer;

    public:
        vector <int*> & getPointer()   { return pointer; }
};

A *a [3];


void f1()
{
    for (int i=0; i < 3; i++)
    {
        cout << "Iteration " << i+1 << ": " << endl;

        for ( vector<int*>::iterator j = a[i]->getPointer().begin();
                                     j != a[i]->getPointer().end();
                                     j++  )
            cout << " Retrieved Data = " << **j <<  endl;

        cout << endl;
    }
}


int main()
{
    for (int i=0; i < 3; i++)
      a[i] = new A;


    for (int i=0; i < 3; i++)
    {
        cout << "Iteration " << i+1 << ":" << endl;
        for (int j=0; j < 5; j++)
        {
            int value =j + i * 10 + 1;
            cout << " Storing Data = " << value << endl;

            int *x = new int;
            *x = value;

            a[i]->getPointer().push_back(x);
        }
        cout << endl;
    }

    cout << endl;
    f1();

    return 0;
}

This code produces correct output, as follows:

Iteration 1:
 Storing Data = 1
 Storing Data = 2
 Storing Data = 3
 Storing Data = 4
 Storing Data = 5

Iteration 2:
 Storing Data = 11
 Storing Data = 12
 Storing Data = 13
 Storing Data = 14
 Storing Data = 15

Iteration 3:
 Storing Data = 21
 Storing Data = 22
 Storing Data = 23
 Storing Data = 24
 Storing Data = 25


Iteration 1: 
 Retrieved Data = 1
 Retrieved Data = 2
 Retrieved Data = 3
 Retrieved Data = 4
 Retrieved Data = 5

Iteration 2: 
 Retrieved Data = 11
 Retrieved Data = 12
 Retrieved Data = 13
 Retrieved Data = 14
 Retrieved Data = 15

Iteration 3: 
 Retrieved Data = 21
 Retrieved Data = 22
 Retrieved Data = 23
 Retrieved Data = 24
 Retrieved Data = 25

Thank you very much !

[activecat]

Just come back to share another way of doing it, which produces exactly same output.


Working code 2: (option B)

#include <vector>
#include <iostream>
using namespace std;


class A
{
    private:
        vector <int*> pointer;

    public:

        A(int quantity)
        {
            for (int i=0; i < quantity; i++)
                pointer.push_back(new int);
        }

        int * getPointer2(int i)
        {
            return this->pointer[i];
        }
};

A *a [3];


void f1()
{
    for (int i=0; i < 3; i++)
    {
        cout << "Iteration " << i+1 << ": " << endl;

        for (int j=0; j < 5; j++)
            cout << " Retrieved Data = " <<  *a[i]->getPointer2(j) <<  endl;

        cout << endl;
    }
}


int main()
{
    for (int i=0; i < 3; i++)
      a[i] = new A(5);


    for (int i=0; i < 3; i++)
    {
        cout << "Iteration " << i+1 << ":" << endl;
        for (int j=0; j < 5; j++)
        {
            int value =j + i * 10 + 1;
            cout << " Storing Data = " << value << endl;

            *a[i]->getPointer2(j) = value;
        }
        cout << endl;
    }

    cout << endl;
    f1();

    return 0;
}

The significant differences between the two options are:

In option A, the integer pointer (produced by new int) is pushed (command: push_back(x)) into pointer array after the integer value is stored into that memory location (refer line 48 to 51 of working code 1);

whereas in option B, the integer pointer is pushed into pointer array at the moment when the instance of A is created (happens in the constructor of A).

[Cubbi]

Don't forget to fix all those memory leaks

==16747== HEAP SUMMARY:
==16747==     in use at exit: 324 bytes in 21 blocks
==16747==   total heap usage: 30 allocs, 9 frees, 492 bytes allocated

[activecat]

Yes, so far memory is allocated by both new A and new int but there is no delete statement. In the real case, objects will only be created at the first part of the program, and then later those objects will be used repeatedly until the program ends. Then the operating system will clear up all allocated memory.

[LB]

You should never rely on the OS to free memory.

Also, you have a problem with your class design - why is external code handling your class private data? Why are you giving almost unrestricted access?

[activecat]

Hi LB,

Thanks for pointing out, but I am not sure of that.
I always assume the OS could nicely free up memory when program exits.
This is called protected operating system.

Meanwhile, the codes above were using int to represent another class (class B) in the real environment. Above is just a simplified code to illustrate the problem.

The code used in actual environment is:

class A
{
    private:
        int quantity_of_B;
        vector < class B * > pointer;
   ....
};

In this case the external code cannot access the class B private data directly, but through B's public member functions.

[LB]

1. The vector keeps track of its own size, you don't need "quantity_of_B"
2. Why do you have A if it is just a wrapper for std::vector<B *>?

[o1vk4iN6]

L B wrote:

You should never rely on the OS to free memory.

What of a problem that requires the program to close immediately ?

[activecat]

Hi LB:

1. The value of "quantity_of_B" needs to be retrieved quite frequently in many loops.
Yes, it could be replaced by pointer.size() but I store explicitly as a variable, for performance.

2. Yes A is just a wrapper of B.
Just imagine class A as "Family", class B as "People".
Then we create many instances of Family, each family has various number of family members (instances of People).
Then of course every people (instance of People) has its own private data.

[activecat]

The program does not suddenly exits.

In the real environment, the number of instances of both class A and class B are always pre-defined before the program starts. So the number of instances does not change dynamically during execution time. And for every instances (either A or B), once it is created, it must not be destroyed except the program ends.

That's why I do not need to delete those class instances, but to let the OS clear up the memory after the program ends.

Is it true that we should never rely on OS to free memory? Maybe this is true for automatic garbage collections in Java etc, but not in my case. Correct?

[jlb]

Why are you even using the pointer, why not just a vector of ints?

Is it true that we should never rely on OS to free memory?

Yes it is true you should never rely on the OS to free your memory. If you use new you should always have a delete.

Also returning a pointer to your private data member allows full access to that item outside of the class, you should be returning a const.

[AbstractionAnon]

Is it true that we should never rely on OS to free memory?

The issue is not whether the OS releases the memory or not. All modern protected OSes will release the memory occupied by the program when the program terminates.

The issues is that it's a poor programming practice not to do so. If you get in the habit of being careless about releasing memory in your program, when you tackle more complex programs where there are lots of objects with different lifespans, you will have memory leaks that will be hard to find and difficult to fix.

Consider a threaded program, or a windowed or event driven program where objects are created upon an event. You want to be very sure that the objects (and memory owned by them) are released and do not create memory leaks or your program may crash due to heap exhaustion.

[LB]

@xerzi Unless it is a very serious issue, in many cases stack unwinding will properly call destructors (which is why a smart pointer should be used).

activecat wrote:

The value of "quantity_of_B" needs to be retrieved quite frequently in many loops.
Yes, it could be replaced by pointer.size() but I store explicitly as a variable, for performance.

In 99% of cases the compiler can inline the call to pointer.size() so that it is just as efficient as caching it. Having a separate size variable is prone to mistakes.

activecat wrote:

Yes A is just a wrapper of B.
Just imagine class A as "Family", class B as "People".
Then we create many instances of Family, each family has various number of family members (instances of People).
Then of course every people (instance of People) has its own private data.

No, in your code A is a wrapper of a std::vector that just so happens to contain B pointers. Why not just use the std::vector directly? Unless A manages itself and no extrnal code performs logic for it, you should just use the std::vector directly. If it makes you feel any better you can use a typedef:
using Bvec = std::vector<B *>;

[activecat]

Thanks. I am now convinced that:

1). the quantity_of_B could be replaced by pointer.size()
2). dynamic memory should be manually cleared up

But for the rest, just look at following code:

#include <iostream>
using namespace std;

class People
{
    public:
        People () { cout << "One people created" << endl; }
};

class Family
{
    public:
        Family( int quantity ) { People people[quantity];  }
};

int main()
{
    int quantity[] = { 2,1,5 };

    for (int i=0; i < 3; i++)
       Family f( quantity[i] );
}

This short code works, but has some fatal issues. But it roughly illustrates the requirements.
How to fulfill the requirements, if not using vector of pointers?

The issues about this code include:
1). The Family instances created (at line 21) is only exists in the scope of the for() loop. How to make them persist outside the for() loop?
2). There must be a way to store the Family instances created.
3). There must be a way to store the People instances created.
4). There must be a way to access People's member functions.

If you could tell me a better way to doing it, rather than using vector of pointers, then I appreciate it.
Thanks.

[MikeyBoy]

1 and 2) Simple - store them in storage that has scope outside of the loop. It doesn't have to be a vector of pointers, although that's the natural way of doing it - why are you opposed to it?

3) Store the array as a data member of the Family class

4) You mean access them via the Family interface? Either (a) proved methods on the Family interface to call through to methods on the People objects, or (b) proved methods on the Family interface to return pointers to the People class. Which you choose is your own design decision - do you want the Family interface to expose the People class, or hide it?

[AbstractionAnon]

1,2 - store your families in a vector.

  vector<Family>  families;

  Family f1;
  //  Initialize f1 somehow
  families.push_back (f1);

There is actually no reason now for your for loop at line 20. Simply create each Family object however you were going to do so, then push it on to the families vector.

3,4 - Isn't a Family a collection of People? Then Family should have a vector of People.

class Family
{  vector<People>  mv_people;  //  mv = member vector
    public:
        Family() {}  
        void AddPerson (Person p)
        { mv_people.push_back (p); }  // Add person to vector
        Person getPerson (int i)
        { return mv_people[i]; }           // return n'th person
};

I've removed quantity from Family's constructor since there is no need for it. Vectors will resize themselves automatically.

[activecat]

Thanks AbstractionAnon.
Your idea is great and it really works !

I've successfully tested it, it works, and to share it here:

File: main.cpp

#include <vector>
#include <cstdlib>
#include <iostream>
using namespace std;

#define familyQTY 3

class People
{
    private:
        int ID;

    public:
        People()
        {
            this->ID =rand();
            cout << "   A Person is created, ID assigned = " << this->ID << endl;
        };

        int getID()  { return this-> ID; }
};

class Family
{
    private:
        vector <People> mv_people;    //  mv = member vector

    public:
        Family(){}
        void AddPerson (People p)  { mv_people.push_back(p);  }
        People getPerson(int i)    { return mv_people[i];     }
        int getQuantity()          { return mv_people.size(); }
};


int main()
{
    int quantity[] = { 2, 1, 5 };

    Family f [ familyQTY ];

    cout << "======= Creating families and their respective members ======="<< endl;
    for (int i=0; i < familyQTY; i++)
    {
        cout << " For the " << i+1 << "th family" << endl;
        for (int j=0; j < quantity[i]; j++)
        {
            People p;
            f[i].AddPerson( p );
        }
        cout << endl;
    }

    cout << endl << "======= Retrieving families and their respective members =======" << endl;
    for (int i=0; i < familyQTY; i++)
    {
        cout << " Status: " << i+1 << "th family has " << f[i].getQuantity() << " family members." << endl;
        for (int j=0; j < f[i].getQuantity(); j++ )
            cout << "    A Person is retrieved, ID = " << f[i].getPerson(j).getID() << endl;
        cout << endl;
    }

    return 0;
}

Note: I don't use vector for the family because the family quantity is known at the very begining.


This code produces following output:

======= Creating families and their respective members =======
 For the 1th family
   A Person is created, ID assigned = 1804289383
   A Person is created, ID assigned = 846930886

 For the 2th family
   A Person is created, ID assigned = 1681692777

 For the 3th family
   A Person is created, ID assigned = 1714636915
   A Person is created, ID assigned = 1957747793
   A Person is created, ID assigned = 424238335
   A Person is created, ID assigned = 719885386
   A Person is created, ID assigned = 1649760492


======= Retrieving families and their respective members =======
 Status: 1th family has 2 family members.
    A Person is retrieved, ID = 1804289383
    A Person is retrieved, ID = 846930886

 Status: 2th family has 1 family members.
    A Person is retrieved, ID = 1681692777

 Status: 3th family has 5 family members.
    A Person is retrieved, ID = 1714636915
    A Person is retrieved, ID = 1957747793
    A Person is retrieved, ID = 424238335
    A Person is retrieved, ID = 719885386
    A Person is retrieved, ID = 1649760492

As compared to the previous one, this approach has the following advantages:
i). It doesn't use pointer at all (hence less dangerous)
ii) This time not using dynamic memory (there is no new statement), hence no need to clear up memory !

Thanks a lot !!

[activecat]

But one thing, isn't that using pointer has advantage too;

When passing class instance as argument to a function, the whole class instance will be copied before being passed around. But if we were using pointer as argument to the function, the class instance won't be copied.

In this case using pointer may cause the program run faster.

Below is one of my project files, just to show that pointers are being used exclusively. I feel in cases like this, pointers makes the program execute faster.

void neural_network_fast()
{
    lesson++;


    InputNode::calc_all_output( inputNode );       // Input nodes: calculate all outputs

    // Hidden Layer-1: feed input
    for (int i=0; i < layer[0]->getNodeQTY(); i++)
        for (int j=0; j < fontWIDTH * fontHEIGHT ; j++)
            layer[0]->getPointer(i)->feed( j, inputNode[j]->get_Output() );  // feed()


    // Hidden Layer-1: calculate all output, self-adjust theta is turned OFF
    for (int i=0; i < layer[0]->getNodeQTY(); i++)
    {
        layer[0]->getPointer(i)->calc_Output();
        //layer[0]->getPointer(i)->self_adjust_theta();
    }

    // -------------------------------------------------------- Hidden Layer-n: forward propagation -------------------------
    for (int k = 1;  k < hiddenLayerQTY; k++)
    {
        // Hidden Layer-k: feed input
        for (int i=0; i < layer[k]->getNodeQTY(); i++)
            for (int j=0; j < layer[k-1]->getNodeQTY() ; j++)
                layer[k]->getPointer(i)->feed( j, layer[k-1]->getPointer(j)->get_Output() );  // feed()

        // Hidden Layer-k: calculate all output, self-adjust theta is turned OFF
        for (int i=0; i < layer[k]->getNodeQTY(); i++)
        {
            layer[k]->getPointer(i)->calc_Output();
            //layer[k]->getPointer(i)->self_adjust_theta();
        }
    }

    // -------------------------------------------------------- Output Nodes  --------------------------------------
    for (int i=0; i < OUTPUTnodeQTY; i++)
        for (int j=0; j < layer[ hiddenLayerQTY-1 ]->getNodeQTY() ; j++)
            outputNode[i]->feed(j, layer[ hiddenLayerQTY-1 ]->getPointer(j)->get_Output() );

    OutputNode::calc_all_output( outputNode );

    bool * binaryAnswer = decimalToBinary( currentCharacter - charBEGIN + 1 );
    feed_correct_answers( outputNode, binaryAnswer );


    if ( lesson % 1000 == 0)
        logIterationDetails();

    // Backword Propagation !
    OutputNode::calc_all_gamma( outputNode );       // calculate gamma


    // Output Node: adjust all weights
    for (int i=0; i < OUTPUTnodeQTY; i++)
        for (int j=0; j < layer[ hiddenLayerQTY-1 ]->getNodeQTY(); j++)
            outputNode[i]->adjust_weight( j, layer[ hiddenLayerQTY-1 ]->getPointer(j)->get_Output() );

    for (int k = hiddenLayerQTY-1; k > 0; k--)   // --------------------------------------------------------
    {
        // Hidden Layer-k: calculate all gamma
        for (int i=0; i < layer[k]->getNodeQTY(); i++)
            if ( k == hiddenLayerQTY - 1 )
                layer[k]->getPointer(i)->calc_gamma( (Node**)outputNode, OUTPUTnodeQTY );
            else
            {
                layer[k]->getPointer(i)->reset_gamma();
                for (int j=0; j < layer[k+1]->getNodeQTY(); j++)
                    layer[k]->getPointer(i)->calc_gamma_manual_step1( layer[k+1]->getPointer(j)->get_gamma() , layer[k+1]->getPointer(j)->getWeight(i) );
                layer[k]->getPointer(i)->calc_gamma_manual_step2();
            }

        // Hidden Layer-k: adjust all weights
        for (int i=0; i < layer[k]->getNodeQTY(); i++)
            for (int j=0; j < layer[k-1]->getNodeQTY() ; j++)
                layer[k]->getPointer(i)->adjust_weight( j, layer[k-1]->getPointer(j)->get_Output() );
    }

    // Hidden Layer-1: calculate all gamma ---------------------------------------------------------------
    for (int i=0; i < layer[0]->getNodeQTY(); i++)
    {
        layer[0]->getPointer(i)->reset_gamma();
        for (int j=0; j < layer[1]->getNodeQTY(); j++)
            layer[0]->getPointer(i)->calc_gamma_manual_step1( layer[1]->getPointer(j)->get_gamma() , layer[1]->getPointer(j)->getWeight(i) );
        layer[0]->getPointer(i)->calc_gamma_manual_step2();
    }

    // Hidden Layer-1: adjust all weights
    for (int i=0; i < layer[0]->getNodeQTY(); i++)
        for (int j=0; j < fontWIDTH * fontHEIGHT; j++)
            layer[0]->getPointer(i)->adjust_weight( j, inputNode[j]->get_Output()  );



}