Is it worth making my code less readable for providing exception safety in case of out of memory errors?

Question

I have a game with non-copyable Items as they are supposed to be unique:

class Item {
  Item() noexcept;
  Item(Item&&) noexcept;
  Item(Item const&) = delete;
// ...
};

class Creature is capable of receiving an Item and adding it to their inventory:

void Creature::receive_item(Item&& item) noexcept {
  this->inventory.push_back(std::move(item));
}

void caller_code() {
  Creature creature;
  Item item;
  // ...
  creature.receive_item(std::move(item));
}

This approach seems nice and clean but there's a small problem: if any part of my code can recover after std::bad_alloc and therefore catches one thrown by recieve_item()'s push_back(), the in-game logic is undefined: an Item was moved to a function which failed storing one so it is just lost. Moreover, the noexcept specifier has to be removed just for this possibility.

So, I can provide exception safety this way (please point it out if I'm wrong):

void Creature::receive_item(Item& item) {
  this->inventory.resize(this->inventory.size() + 1);
  // if it needs to allocate and fails, throws leaving the item untouched
  this->inventory.back() = std::move(item);
}

void caller_code() {
  Creature creature;
  Item item;
  // ...
  creature.receive_item(item); // no moving
}

However, now we have new disadvantages:

• the lack of std::move() in the caller's code obscures the fact of moving the item;
• the "resize(+1)" part is just ugly and may be misunderstood during a code review.

The question is in the title. Is exception safety even for such a weird case considered a good design?

Answer 1

The answer to your question depends on whether you can and want to handle memory allocation errors, other than by crashing. If you do, you will have to implement measures beyond just catching std::bad_alloc. Most modern operating systems implement memory overcommit, which means memory allocation will succeed, but accessing the allocated memory for the first time will cause a page fault, which will normally result in a crash. You have to explicitly fault allocated pages in your memory allocator to detect out of memory condition before returning the pointer to the caller.

Regarding your code modification, you don't have to modify your call to push_back:

this->inventory.push_back(std::move(item));

If push_back (or any other potentially reallocating method) needs to allocate a new buffer, it will do this before moving the new item into the vector. Obviously, this is because there is no room in the vector to move the new element to. And when the buffer is reallocated and all existing elements are moved/copied to the new buffer, the new element is moved as the final step.

In other words, if push_back throws, you can be sure the new item has not been moved. If it doesn't throw and returns normally, the item is moved from.