Search code examples
c++listpointersiteratorauto

Incorrect values with auto iterator on std::list

I firstly intialize a hotel object and then initialize some room objects and print their id's respectively (which print out correctly.)

for(int j=0;j<5;j++){
    Room r(1, 30);
    hotel.addRoom(r);
    cout << "Id: " << r.getId() << endl;
}

Then, I do this iteration on the list:

cout << "Initialized rooms with Ids: ";
for(auto iterator : hotel.getRooms()){
    cout << iterator->getId() << " ";
}
cout << endl;

Also, here is the implementation on those methods, on the Hotel class:

//header
list<Room*> rooms;

//source
list<Room*> & Hotel::getRooms(){
    return rooms;
}

And look at the output! Output

Every other part (like id generation and construction of objects) is tested and works fine.

Solution

  • Since you haven't posted a definition for the addRoom function, by the power of deduction I conclude it does something similar to:

    void Hotel::addRoom(const Room& room) {
    rooms.push_back(&room);
}

    And then given the following loop:

    for(int j=0;j<5;j++)
{
    // This creates a Room object local to each iterator of the loop
    Room r(1, 30);
    // This adds the address of this local variable in the list
    hotel.addRoom(r);
    cout << "Id: " << r.getId() << endl;
    // The variable r is destroyed here
}

    So in the end your std::list<Room*> is filled with dangling pointers - pointers which point to memory which is no longer yours. This causes undefined behavior.

    I recommend you drop the pointers altogether and change to:

    class Hotel 
{
    std::list<Room> rooms;

public:
    void addRoom(const Room& room) {
        rooms.push_back(room);
    }
    std::list<Room>& Hotel::getRooms() {
        return rooms;
    }
    const std::list<Room>& Hotel::getRooms() const {
        return rooms;
    }
};