3
\$\begingroup\$

I'm trying to learn C++ again, trying to do all the "modern" stuff (e.g., avoid pointers in favor of references) and I found that I have some trouble understanding how references and lifetimes work. I'm using clang with -std=c++11.

I want to have a RAII-class, RoomList that basically wraps a map<int, Room>. On construction of RoomList, it will create all the Rooms (imagine loading them from a data file or so) and the map, and on destruction it should free the map and all Rooms.

This code works, but I don't fully understand all of it (I've done a lot of "randomly trying stuff" to get it to work). (GIST is here)

The Room class:

class Room {
public:
  explicit Room(int id, const std::string& name) : m_id(id), m_name(name) {}
  const std::string& GetName() const { return m_name; }
  const int& GetEnterCount() const { return m_enterCount; };
  void OnEnter() { ++m_enterCount; }
private:
  std::string m_name; // should this be const?
  int m_id; // should this be const?
  int m_enterCount = 0;
};

The RoomList class:

class RoomList {
public:
  RoomList();
  ~RoomList() = default; // Is this needed? Rule of three?
  RoomList(const RoomList&) = delete; // Don't want copies
  RoomList& operator=(RoomList const&) = delete;
  const Room& GetRoom(int index);
private:
  // Should this be <int, Room> or <int, Room&>?
  std::map<int, Room> m_rooms;
};

RoomList::RoomList() {
  // Read all the rooms, e.g., from a data file
  // What is the lifetime of the Room I'm creating?
  // Room doesn't have copy or move ctors
  // Also, I'm not initializing m_rooms - what is its lifetime?
  m_rooms.emplace(0, Room(0, "Start Room"));
}

// Returning a reference to const Room - no one should be able to modify Room, except by calling room.OnEnter()
// room.OnEnter() should modify the Room inside the map (Individual Rooms are basically Singletons)
const Room& RoomList::GetRoom(int index) {
  auto result = m_rooms.find(index);
  if(result != m_rooms.end())
  {
    // result is a local var - is result->second valid after return?
    return result->second;
  }

  // Considering Optional<Room>
  // https://github.com/akrzemi1/Optional/
  throw "Invalid Index";
}

main.cpp:

int main() {
  RoomList rooms;
  auto room = rooms.GetRoom(0);
  std::cout << "Name: " << room.GetName() << std::endl;
  return 0;
}

I left some comments, but basically the things I'm confused about:

  • I'm not initializing the m_rooms map - will it be default initialized to live as long as the rooms variable in main.cpp is in scope?
  • I have a map of Rooms, but I want to return Room references. Will returning a Room have any negative side effects? (e.g., is there some automatic creation of a wrapper object or even a copy of Room going on?)
  • Is result->second even still valid after a return? result is a local variable that should die when the function ends, but I'm returning what second points to, so that should be good?
  • Would I need to implement ~RoomList to make sure all the Rooms are freed? Or does std::map::emplace own the constructed Room?
  • If I wanted to return something by value, how would I need to allocate it? For example, imagine I've added this to Room, wouldn't I return a stack-allocated variable?

This code works, but I think it shouldn't:

class Room {
public:
  // ...
  std::string GetRoomDescription();
}

std::string Room::GetRoomDescription()
{
  // What's the lifetime of "result"?
  // In C, this would be stack-allocated and dead
  // when the function returns.
  std::stringstream result;
  result << "ID: " << m_id << " - Name: " << m_name;
  return result.str();
}

// main.cpp
std::cout << "Room: " << room.GetRoomDescription() << std::endl;
\$\endgroup\$
  • \$\begingroup\$ There is a difference between your main here, and the main in your gist, that actually makes quite a difference. \$\endgroup\$ – Zulan Nov 4 '15 at 12:09
  • \$\begingroup\$ @Zulan Indeed, it misses the const-ness issue of GetRoom. I've just experimented with some of the recommendations in your answer and ran into that. I didn't consider const-ness important (i was more worried about lifetime) but now that this stuff makes more sense I see the issue. The class design needs major tweaking to separate concerns, after I've figured out how C++ ownership/lifetime works. \$\endgroup\$ – Michael Stum Nov 4 '15 at 12:30
  • \$\begingroup\$ What's RAII about this code? There's no resource being acquired. \$\endgroup\$ – Barry Nov 4 '15 at 15:14
  • \$\begingroup\$ @Barry Not sure if I'm using the term correctly, but the intent is that RoomList owns all the Rooms, and that the Rooms are alive as long as RoomList is alive. \$\endgroup\$ – Michael Stum Nov 4 '15 at 18:17
4
\$\begingroup\$

There are some issues here:

  auto room = rooms.GetRoom(0);

Here you do copy the Room because auto does not imitate the 'reference part'. To bind a reference to the internal map entry, you need to both return by reference and assign to a reference type. Based on your gist, I assume you would think that if you then call room.OnEnter, the Room is also updated in the RoomList. This is not the case, because it is a copy. Therefore you would want to use:

  auto& room = rooms.GetRoom(0);

instead. But for that to work, RoomList::GetRoom should return a non-const reference, so that you can actually modify it. In turn there should also be a const RoomList::GetRoom that also returns a const reference.

However, there should also be a non-const RoomList::GetRoom that returns a non-const reference, so that you can actually call non-const methods on the returned rooms.

To avoid running into that issue - you might want to delete the copy constructors and assignment operators of Room. If you further wanted to enforce that all Room objects are owned only by RoomList you could even make the Room constructor private and friend RoomList.

I'm not initializing the m_rooms map - will it be default initialized to live as long as the rooms variable in main.cpp is in scope?

Yes

I have a map of Rooms, but I want to return Room references. Will returning a Room have any negative side effects? (e.g., is there some automatic creation of a wrapper object or even a copy of Room going on?)

Not for std::map. You can delete the copy constructor and assignment operator or Room, that will ensure you make no copies somewhere else in the code also.

See https://stackoverflow.com/a/6442829/620382 for a listing of iterator/reference invalidations in standard containers.

Is result->second even still valid after a return? result is a local variable that should die when the function ends, but I'm returning what second points to, so that should be good?

Yes. result points to the internal value_type of the map.

Would I need to implement ~RoomList to make sure all the Rooms are freed? Or does std::map::emplace own the constructed Room?

RoomList::m_rooms own the constructed Rooms. More technically speaking:

m_rooms.emplace(0, Room(0, "Start Room"));

Here a temporary Room object is created, which is then moved into the std::pair<int, Room> that is construed inside of the map. In C++14 you can use std::piecewise_construct to even skip the temporary Room object. But as long as you have a move constructor, which you have by default, you are fine.

If I wanted to return something by value, how would I need to allocate it? For example, imagine I've added this to Room, wouldn't I return a stack-allocated variable?

If you do not declare the return value to be a reference (which you do not in your example), then it is returned by copy. But the C++ rules and compilers are good so that you will either get a move (cheaper than actual copy) or even omitting it alltogether when possible.

// Considering Optional

If you document it properly, it would be actually fine to return a raw pointer here, with nullptr meaning that it is not available. The raw pointer (similar to the raw reference) does not imply ownership. You just need to make sure the caller does never reference it without checking. optional (also available in boost and soon in the standard).

The funny thing about your code is, that you have randomly written proper and safe code. IMHO this is a nice property especially about modern C++. There is actually no explicit resource allocation (like new) in your code - when you allocate resources implicitly, through member variables, C++ will behave nicely by default and also implicitly release the resources.

  std::string m_name; // should this be const?
  int m_id; // should this be const?

Yes, it would be a good idea to make those const.

const int& GetEnterCount() const { return m_enterCount; };

I would consider it a uncommon to return an int by const reference here (as opposed to a simple copy). I don't have a strong argument for or against, but lets just take note that std::vector::size doesn't return an const int& either.

  RoomList& operator=(RoomList const&) = delete;

You should put the const before RoomList for consistency. You also might want to explicitly default or delete the move constructor and assignment operators. I would say neither would hurt.

In Room, you list m_id and m_name in a different order in the initializer list. This can lead to confusion because the initialization order follows the order from member declaration (m_name then m_id). Therefore this is a compiler warning.

So the tip is, compile with maximal warnings, e.g. -Wall -pedantic for gcc. Also it can be helpful to run your code with valgrind to check for any leaks.

\$\endgroup\$
3
\$\begingroup\$

explicit only makes sense on single-argument-constructors and on conversion operators, but Room::Room(int id, const std::string& name) has two arguments...

Room has 4 implicitly defined special-member-functions: Copy- and Move- Ctor and assignment.
Luckily, as all members representing additional resources (the std::string) use RAII, they do the right thing.
At least if copying a room makes any kind of sense, which I doubt.

If you make a member const, all but the copy-constructor would be deleted...


Why don't you allow moving a RoomList? As an aside, explicitly defaulting the move-constructor and/or move-assignment-operator in the class will disable implicit generation of copy-ctor and copy-assignment-operator.

There is only one reason to explicitly default a destructor in-class: To mark it virtual.
As an aside, if a special-member-function is user-declared but not defaulted in-class, or any of the base-classes versions aren't trivial, it cannot be trivial.

If you upgrade to C++14, your list of rooms should not be a std::map but a std::set, and Room should be comparable according to the room-number, and comparable with numbers.

There's a better way to put Rooms in your list:

m_rooms.emplace(std::piecewise_construct,
    std::forward_as_tuple(0),
    std::forward_as_tuple(0, "Start Room"));

In C++17, there will be try_emplace...

You were right that you must not return a local variable. But that's not what you are doing anyway, you are returning a reference to the object the local variable (an iterator) points to, which is harmless.

Be aware that return 0; is implicit in main.

Your last snippet is ok, the temporary created by result.get() gets moved (though RVO applies, so it actually gets substituted) to the functions return-value.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.