13

I have some code that adds to a std::vector and a std::map after creating an object.

v.push_back(object);     // std::vector
m[object->id] = object;  // std::map

I want to make this have a strong exception guarantee. Normally, to make operations like these atomic, I would implement a swap method for each container, and call all of the functions that could throw on temporary copies of the container:

vector temp_v(v);
map temp_map(m);

temp_v.push_back(object);
temp_m[object->id] = object;

// The swap operations are no-throw
swap(temp_v, v)
swap(temp_m, m)

However, making temporary copies of the entire vector and map seems very expensive. Is there any way to implement a strong exception guarantee for this function without the expensive copies?

Kai
  • 2,482
  • 1
  • 19
  • 21
  • What kind of exceptions are you expecting to catch and where? The only thing that would cause either `v.push_back(object);` or `m[object->id] = object;` to throw would be the copy constructor/assignment operator of `object`. Well that or `std::bad_alloc`, but in the event of such things, the objects will still be cleaned up. This is required by the spec. – Nicol Bolas Jan 13 '12 at 21:53
  • Can copy constructor of object throw an exception? – Oleksandr Pryimak Jan 13 '12 at 21:54
  • @NicolBolas: I think his main concern is that the vector is in a "broken" state when inserting into the map throws. – PlasmaHH Jan 13 '12 at 21:55
  • @AlexandrPriymak: Sure. Every function can. The question though is about it being a good idea. For object creation, throwing in a ctor (no matter if copy, move or "normal") is a common thing to signal object creation failure. Not to talk about `std::bad_alloc` (in the case you care about `new` throwing at all). – PlasmaHH Jan 13 '12 at 21:56
  • @PlasmaHH: Throwing in a move ctor is *not* a common thing. It's absolutely destructive to the exception safety of any and all containers to do so. I think that move constructors and assignment operators are even implicitly declared `noexcept`, but I could be wrong. – Xeo Jan 13 '12 at 21:57
  • @Xeo: When a move ctor can fail, it is a common thing to signal failure for it via throwing too. It is just that its uncommon for move ctors to be able to fail. – PlasmaHH Jan 13 '12 at 21:58
  • 2
    @PlasmaHH: A failing move ctor is in the same end-time scenario as a failing resource destruction (aka throwing from a dtor) - *it must not exist*. – Xeo Jan 13 '12 at 22:01
  • 1
    @Xeo - non-throwing move constructors were present in the initial proposal, but this was later relaxed and they now *are* allowed to throw. The vector will, in some cases, fall back to copying when detecting a potentially throwing move. See the `move_if_noexcept` helper in the C++11 standard library. – Bo Persson Jan 14 '12 at 05:01
  • @BoPersson: IMHO, if the move constructor eventually throws, you shouldn't have one, and write a copy ctor instead. – Xeo Jan 14 '12 at 05:03
  • 1
    @Xeo - Throwing move constructors are surely unusual, and generally not very useful, but also not forbidden by the language. So the containers will have to account for them. – Bo Persson Jan 14 '12 at 05:08
  • @BoPersson: Yes, but why make the life of you and others harder than it already is? ;) If containers will resort to copying anyways, it's a good indication that you should actually have a copy ctor, not a move ctor. I'd really like to know a situation where a move ctor could possibly fail, if it indeed actually *moves* something and is not actually a glorified copy ctor. – Xeo Jan 14 '12 at 05:11

4 Answers4

4

General Case

Technically, only one copy is required:

  1. Copy the vector
  2. Update the copy
  3. Update the map
  4. Swap the copy and the original vector

Another option is catch-roll-back-and-rethrow:

  v.push_back(object);
  try
  {
    m.insert(object->id, object); // Assuming it cannot be present yet
  }
  catch(..)
  {
    v.pop_back();
    throw;
  }

Or the other way around. I picked this order because the vector::pop_back() is guaranteed not to fail.

UPDATE: If object->id could be present, see Grizzly's answer for a solution.


Specific Case for Pointers

However, as you are using object->, you might be storing pointers. The copy-constructor of a pointer cannot throw, and we can use that fact to simplify the code:

v.reserve(v.size() + 1);
m[object->id] = object; // can throw, but not during the assignment
v.push_back(object); // will not throw: capacity is available, copy constructor does not throw

And if you are really worried about frequent resizing:

if (v.capacity() == v.size()) v.resize(v.size()*2); // or any other growth strategy
m[object->id] = object;
v.push_back(object);
Community
  • 1
  • 1
Sjoerd
  • 6,837
  • 31
  • 44
  • 1
    What happens when `operator[]` call succeeds, but the assignment to its result throws? – PlasmaHH Jan 13 '12 at 22:00
  • @PlasmaHH hmm... you're right, that's a valid case. Let's switch to `m.insert(object->id, object);` – Sjoerd Jan 13 '12 at 22:05
  • @PlasmaHH Assignment never fails because of the fact that object is a pointer – Oleksandr Pryimak Jan 13 '12 at 22:06
  • 1
    @Sjoerd: Sorry to nitpick, but this is a different behaviour, as when the object is already there, the op[] version overwrites, whereas the insert() version will not. You probably want to insert, and save the result. When the result signals insert failure (due to value already being there) then you use the returned iterator to overwrite. I think only then there is no path where an exception could leave the map in an inconsistent state. – PlasmaHH Jan 13 '12 at 22:08
  • @AlexandrPriymak: Is it? I can't see the OP stating that. Just because someone is using `operator->` in his code doesn't mean it is a raw pointer. It could also be some kind smart pointer. It could also as well be that the OP didn't think about the actual syntax he is using there. Naming the variable `object` suggests that it is an actual copy. We should wait for the OP to comment on that. – PlasmaHH Jan 13 '12 at 22:11
  • @PlasmaHH Yes, you are right. In practice, this check usually is already present in the code before this block, but that's just an assumption. I took the easy way out by adding a comment 'Assuming it does not exist yet' ;) – Sjoerd Jan 13 '12 at 22:15
  • @Sjoerd: +1, the important thing is to be clear in communicating that the issue exists. – PlasmaHH Jan 13 '12 at 22:17
  • I didn't mean for this discussion to be about pointers specifically, it's just incidental to the example. Although I'm glad that it brought up good points about the assignment operation failing. – Kai Jan 13 '12 at 22:19
4

I think this is a situation where using try-catch is the correct manner of handling it. If the access to the map throws you undo the operation on the vector and rethrow:

v.push_back(object);  
try 
{
  m[object->id] = object;
} 
catch(...)
{
  v.pop_back();
  throw;
}

However this will still not give you a strong guarantee, since operator[] on maps is a problematic instruction for exception safety (if the element is not in the map an object is default constructed, which will stay in the map, if the operator= throws (very unlikely in this case, since you seem to be storing pointers, but still).

Therefore I would rewrite it as

try 
{
  auto iter = m.find(object->id);//used auto for shorter code,
  if(iter == m.end())
    m.insert(std::make_pair(object->id, object);
 else
   iter->second = object;
}
Grizzly
  • 19,595
  • 4
  • 60
  • 78
4

You can use a scopeguard object that rolls back the operations when destroyed unless it is told not to. This approach is outlined in Generic: Change the Way You Write Exception-Safe Code — Forever.

E.g. something like:

container1.push_back(a);
Guard g(bind(&ContainerType::pop_back, &container1));
container2.push_back(a);
// ...
g.dismiss();
Georg Fritzsche
  • 97,545
  • 26
  • 194
  • 236
  • I like the article you linked. The sample code your wrote seems to suggest the Guard can accumulate multiple undo actions. How does it manage that without risk that Guard::add itself may throw? – Adrian McCarthy Jan 13 '12 at 23:55
  • @AdrianMcCarthy: Good point, that would become a more intrusive wrapping of the insertions `g.add(actualOperation, undoOperation)` and less readable in comparison to their macro. – Georg Fritzsche Jan 14 '12 at 00:26
2

I suppose you could roll your own RAII-type object:

template<typename T>
class reversible_vector_pusher
{
  private:
    std::vector<T> * const ptrV;
    bool committed = false;
  public:
    reversible_vector_pusher(std::vector<T> & v, const T & obj) : ptrV(&v)
        { v.push_back(obj); }
    void commit()
        { committed = true; }
    ~reversible_vector_pusher()
    {
        if(! committed)
             ptrV->pop_back();
    }
};



reversible_vector_pusher<...> rvp(v, object); // replace ... with object's type
m[object->id] = object;
rvp.commit();

(I chose to reverse the vector-push because it's always reversible, whereas with a map you might have overwritten another element that you'd have to try to get back.)

ruakh
  • 175,680
  • 26
  • 273
  • 307