-2

I am trying to create object that will store inside vector using pointers

I can store object inside vector when I don t use pointers, but when I try to use pointers I can not do that

// created class that combine some attributes for file "datum" = date,     "vrijeme" = creation of file etc.

class datoteka{
    public:
      string datum;
      string vrijeme;
      string velicina;
      string ime;

    datoteka();
    datoteka(string datum, string vrijeme, string velicina, string ime)

    {
        this -> datum = datum;
        this -> vrijeme = vrijeme;
        this -> velicina = velicina;
        this -> ime = ime;
    }

    ~datoteka();
};


int main()
{
    vector <datoteka> fajlovi;

    string linija;
    string prva;
    int i = 0;
    datoteka * pokObjDatoteke;
    pokObjDatoteke = new datoteka();

    std::ifstream pisi("list.txt"); //file is open 

    while(getline(pisi,linija)) //get first line of file 
    {
        string vrijednost; 
        stringstream red;  //create stream from line 
        string datoteka[4]; // create array to store seperate information
        red << linija; 
        while(!red.eof() && i != 4) // since line containt lot of tabs i                          
                                    read just first for values
        {
            red >> vrijednost;
            datoteka[i]= vrijednost;
            i++;
            cout << vrijednost << " ovo je vrijednost" << endl;
        }
        pokObjDatoteke->datum = datoteka[0];
        pokObjDatoteke->vrijeme = datoteka[1];
        pokObjDatoteke->velicina = datoteka[2];
        pokObjDatoteke->ime = datoteka[3];

        fajlovi.push_back(*pokObjDatoteke);  /**** problem ****
    }

    return 0;
}

I want to store objects in vector, instead I get just some memory location, probably from pointer but how can I store object and not address

explorer
  • 61
  • 7
  • 6
    What do you mean by "cannot do that" and "problem"? What outcome do you expect, and what different outcome do you observe instead? I don't see anything fatally wrong with this code. While it's unnecessary to make `pokObjDatoteke` a pointer and allocate it on the heap, it's mostly harmless (you do leak one `datoteka` object). – Igor Tandetnik Jun 21 '19 at 12:53
  • Stop messing around with `new` (and `delete`). Just use `datoteka pokObjDatoteke;`. I can't see any reason why you would need a pointer there. – πάντα ῥεῖ Jun 21 '19 at 12:56
  • 1
    Your code is unnecessarily complicated. The local `string datoteka[4];` is unnecessary, you can read right into `pokObjDatoteke->datum` etc. And as a matter of style and readability I would not name variables like types. I didn't even know that's possible. – Peter - Reinstate Monica Jun 21 '19 at 13:04
  • Another thing is that as a matter of principle you should use English names in programs. English is the *lingua franca* in IT -- everybody understands a `ptr` prefix, but not everybody understands `pok`. The times that we worked in isolated pockets of people speaking the same fringe language are over, as you are witnessing right now. (The world would be a better place if everybody used 7 bit ASCII and English, but nobody is listening to me.) – Peter - Reinstate Monica Jun 21 '19 at 13:10
  • 1
    What is the "problem"? Do you observe a compilation error? Runtime error? – Igor R. Jun 21 '19 at 13:28
  • I am learning c++ so I am trying to compare those two methods. As I read from several sources when I use object creation like "datoteka MyFile; " then it s created in STACK and since I want to create vector of several hundred objects of class datoteka, it s better to use HEAP and then it s better to use pointers? correct ? – explorer Jun 21 '19 at 15:52
  • @explorer Your concern is misplaced, vectors don't store their contents on the stack, even if the vector itself is on the stack. And in any case what is wrong with the stack? It's more efficient than the heap. – john Jun 21 '19 at 17:06
  • A `vector ` always stores *entire distinct datoteka objects* which in your case are copies of the various values `*pokObjDatoteke` assumes: `push_back(*pokObjDatoteke)` implicitly invokes the copy constructor `datoteka::datoteka(const datoteka&)` with `*pokObjDatoteke` (which is an object) as the constructor's argument. (You can test that by providing an explicit copy ctor which writes something to stderr.) – Peter - Reinstate Monica Jun 21 '19 at 18:08
  • So if I understand correctly no matter how I create objects, vector create copy of object in heap? And can I think about STACK as smaller amount of memory, but little faster (since data are stored in sequence) then HEAP and HEAP is bigger amount of memory but slower since data are more fragmented. I am trying to correctly understand how this works :) – explorer Jun 22 '19 at 06:04

3 Answers3

1

This:

fajlovi.push_back(*pokObjDatoteke);

stores a copy of the object pokObjDatoteke points to in the vector. The pointed-to object is then leaked, since you never delete it.

Do you really need pointers here though? Your vector contains value types, not pointers, so just use emplace_back to construct your objects directly in the vector:

while (getline(pisi, linija))
{
    string vrijednost; 
    stringstream red;
    string datoteka[4];
    red << linija; 
    while (!red.eof() && i != 4) {
        red >> vrijednost;
        datoteka[i]= vrijednost;
        i++;
        cout << vrijednost << " ovo je vrijednost" << endl;
    }
    fajlovi.emplace_back(datoteka[0], datoteka[1], datoteka[2],
                         datoteka[3]);
}

However, if you really want pointers in your vector, then declare it as such:

vector <datoteka*> fajlovi;

and then change the push_back call in your original code to:

fajlovi.push_back(pokObjDatoteke);

This will push the pointers into the vector. You need to remember to delete them when the vector is destroyed, or else all the objects will get leaked:

for (auto* obj : fajlovi) {
    delete obj;
}

Manual memory management like this is error-prone and a headache. Don't do it. Use smart pointers instead, like shared_ptr. Remove these lines in your original code:

vector <datoteka> fajlovi;

// ...

datoteka * pokObjDatoteke;
pokObjDatoteke = new datoteka();

and replace them with:

#include <memory>
// ...

vector <shared_ptr<datoteka>> fajlovi;
// ...

auto pokObjDatoteke = make_shared<datoteka>();

and then push_back with:

fajlovi.push_back(pokObjDatoteke);

No need to call new or delete.

However, again, it does not look like you need pointers at all. You can just use value types as shown in the beginning of this answer.

Nikos C.
  • 47,168
  • 8
  • 58
  • 90
  • Have my upvote, but allow me to remark that on no contemporary standard OS an object allocated in `main` would leak. The OS simply reclaims a program's memory when the program exits (which is triggered by returning from `main`). A delete at the end of main would have no benefit whatsoever; in particular, neither the program could use it (unless static destructors need more memory than its running state) nor the OS (since usually the C++ runtimes do not return freed memory to the OS). If anything, freeing has educational value, but not freeing simply does not leak here in any meaningful sense. – Peter - Reinstate Monica Jun 21 '19 at 18:01
0

If you want to store objects, then why are you using pointers?

datoteka pokObjDatoteke;

while (...)
{
    ...
    pokObjDatoteke.datum = datoteka[0];
    pokObjDatoteke.vrijeme = datoteka[1];
    pokObjDatoteke.velicina = datoteka[2];
    pokObjDatoteke.ime = datoteka[3];

    fajlovi.push_back(pokObjDatoteke);
}

Why are you trying to use pointers when, like you said, not using pointers works?

john
  • 71,156
  • 4
  • 49
  • 68
0

While the OP didn't disclose an example of the input file, this line (besides all the issues pointed out in the other answers) seems suspicious

while(!red.eof() && i != 4) {...}

See e.g. Why is iostream::eof inside a loop condition (i.e. `while (!stream.eof())`) considered wrong?

A better pattern is to perform the extraction and then check if it was successful.

while( some_stream >> some_data ) {...}

OP's code could be rewritten into something like this

#include <iostream>
#include <fstream>
#include <string>
#include <sstream>
#include <vector>

class datum
{
    std::string a_, b_, c_, d_;
public:
    datum() = default;
    datum(std::string const &a, std::string const &b, std::string const &c, std::string const &d)
        : a_(a), b_(b), c_(c), d_(d)
    {}

    friend std::istream &operator>> (std::istream &is, datum &obj)
    {
        return is >> obj.a_ >> obj.b_ >> obj.c_ >> obj.d_;
    }
    friend std::ostream &operator<< (std::ostream &os, datum const &obj)
    {
        return os << "a: '" << obj.a_ << "', b: '"<< obj.b_
                  << "', c: '" << obj.c_ << "', d: '" << obj.d_ << '\'';
    }
};

int main()
{
    std::vector<datum> data;
    std::ifstream in_stream {"list.txt"};
    if ( !in_stream )
    {
        std::cout << "Error: unable to open input file.\n";
        return -1;
    }

    std::string line, junk;
    while(getline(in_stream, line))
    {
        // This won't consider an error to have empty lines in the file 
        if (line.empty())
            continue;

        std::istringstream iss(line);
        datum dd;
        iss >> dd;
        // Stops if there are more or less strings than required in the line
        if ( !iss  or iss >> junk)
            break;
        data.push_back(std::move(dd));
    }

    for (auto const & d : data)
        std::cout << d << '\n';
}

Testable here.

Bob__
  • 9,461
  • 3
  • 22
  • 31