0

I know that this question is similar to others, but I looked here and here and neither of these solved my problem. Everything I have been able to find says that the following code should work. The vector I use for testing has elements with names superman, batman, and spiderman. I can't remove any more code for the example to make sense I don't think. 

class Room{
  vector<User> users;
  users = {{superman, pass}, {batman, pass}, {spiderman, pass}};

public:
  vector<User> Room::getUsers() {return users;}
};

class User{
  string username;
  string pass;
};



    // Find the room. Once found, scan through the users
    // Once the user is found, remove them from the vector
    for(roomIt = rooms.begin(); roomIt < rooms.end(); roomIt++) {
      if (roomIt->getName().compare(roomName) == 0) {
        for (useIt = roomIt->getUsers().begin(); useIt < roomIt->getUsers().end(); useIt++) {
          if (useIt->getName().compare(username) == 0) {
            useIt = roomIt->getUsers().erase(useIt);
            const char * msg = "OK\r\n";
            write(fd, msg, strlen(msg));
            return;
          }
        }
      }
    }

The room object has a vector of type user. roomIt->getUsers() returns the vector of users in a room. The innermost if statement doesn't do anything, however. To debug, I have a vector of 3 Users. When using gdb, I can see that useIt = roomIt->getUsers().erase(useIt) causes the iterator to move from the second element in the vector to the third element. However, the vector does not change. There were 3 elements beforehand, and there are 3 elements afterwards as well.

Stephen Burns
  • 162
  • 2
  • 17
  • 3
    What does `getUsers()` return? i.e [mcve] please – NathanOliver Apr 10 '18 at 21:17
  • 1
    Sorry, not definitely answerable without complete code. My guess would be that `getUsers()` returns a copy instead of a reference. – user2328447 Apr 10 '18 at 21:19
  • My crystal ball is telling me you're returning a copy of the vector, and not a reference to the original vector. – PaulMcKenzie Apr 10 '18 at 21:20
  • You should consider to use https://en.wikipedia.org/wiki/Erase%E2%80%93remove_idiom – Slava Apr 10 '18 at 21:27
  • 1
    Works on my machine. I had to add the missing code. The code I added must not have the bug that your code has. Can you provide a minimal, complete, verifiable example? – Eljay Apr 10 '18 at 21:31
  • @Eljay I've edited the code to add the bare minimum functions and objects that I can provide. Are the other users correct in stating that I am using a copy, not a reference, which is creating the problem? – Stephen Burns Apr 10 '18 at 21:50
  • @StephenBurns Yes, you're returning a copy of the vector instead of a reference. Surprised your program didn't die due to invalid iterator usage. – PaulMcKenzie Apr 10 '18 at 21:57
  • Do not put the "`Room::`" on a member declaration inside the class definition. It is incorrect C++ syntax, although MSVC allows it. – aschepler Apr 10 '18 at 22:22

1 Answers1

1

As user2328447 speculated in the comments your getUsers is returning by value, not reference. So every single time you call it, you get a completely unrelated vector (same values, but stored separately); even your loop construct is wrong (it's comparing the .begin() from one copy of the vector to the .end() of a separate copy).

If you want to be able to mutate the instance's vector from the caller, you must return a reference, changing:

vector<User> Room::getUsers() {return users;}

to:

vector<User>& Room::getUsers() {return users;}
            ^ return reference, not value
ShadowRanger
  • 143,180
  • 12
  • 188
  • 271
  • 1
    Additional suggestion: Decide whether code which uses class `Room` should be able to modify the user list via `getUsers()`. If no, make the declaration `const vector& getUsers() const;`. If yes, implement both that `const` version (so code can still view users from a `const Room&`) and also a `vector& getUsers();`. – aschepler Apr 10 '18 at 22:18
  • @aschepler: As written, they definitely need to be able to modify the user list. But yes, providing a version returning a `const` reference is a good idea for cases where the caller only needs to see the user list, not `erase` from it. – ShadowRanger Apr 11 '18 at 00:40