EricWF accepted this revision. This revision is now accepted and ready to land.
LGTM modulo inline comments. ================ Comment at: include/map:1138 @@ +1137,3 @@ + else + return emplace_hint(__p, + _VSTD::piecewise_construct, _VSTD::forward_as_tuple(__k), ---------------- mclow.lists wrote: > I think this should be `__h`, not `__p`, since `__p == end()` I actually think you should leave this as is. `__tree` seems to properly handle the end iterator as a hint. `__tree::__find_equal(hint, ...)` assumes that if `hint` is `end()` then the value should be inserted just prior to `hint`. You should be able to implement the the `try_emplace(hint, ...)` methods in terms of the normal `try_emplace` methods because we ignore the hint anyway. ================ Comment at: test/std/containers/associative/map/map.modifiers/try.emplace.pass.cpp:70 @@ +69,3 @@ + for ( int i = 0; i < 20; i += 2 ) + m.emplace ( i, Moveable(i, (double) i)); + assert(m.size() == 10); ---------------- Why not use `try_emplace` here as well? ================ Comment at: test/std/containers/associative/map/map.modifiers/try.emplace.pass.cpp:73 @@ +72,3 @@ + + Moveable mv1(3, 3.0); + r = m.try_emplace(2, std::move(mv1)); ---------------- For the sake of covering corner cases could we test every inserted value? This applies for `insert_or_assign` as well. ``` for (int i=0; i < 20; i += 2) { Movable mv1(i, (double)i); r = m.try_emplace(2, std::move(mv1)); assert(m.size() == 10); assert(!r.second); // was not inserted assert(!mv1.moved()); // was not moved from assert(r.first->first == 2); // key } ``` ================ Comment at: test/std/containers/associative/map/map.modifiers/try.emplace.pass.cpp:80 @@ +79,3 @@ + + r = m.try_emplace(3, std::move(mv1)); + assert(m.size() == 11); ---------------- We should test the cases where insertion happens at the front and at the back. http://reviews.llvm.org/D10669 _______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits