From 2132abb65cc5875620a91bcb32ef0fbe90096aff Mon Sep 17 00:00:00 2001 From: vng Date: Thu, 22 Apr 2021 19:37:39 +0300 Subject: [PATCH 1/3] Fixed bug with buffer_vector::push_back. Signed-off-by: vng --- base/base_tests/buffer_vector_test.cpp | 16 ++++++++++++++++ base/buffer_vector.hpp | 2 +- routing/index_graph_starter_joints.hpp | 2 +- 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/base/base_tests/buffer_vector_test.cpp b/base/base_tests/buffer_vector_test.cpp index 1588bf0123..789e1ff7a2 100644 --- a/base/base_tests/buffer_vector_test.cpp +++ b/base/base_tests/buffer_vector_test.cpp @@ -17,6 +17,19 @@ namespace } } +UNIT_TEST(BufferVector_PushBack_And_Realloc) +{ + using ElementT = std::vector; + ElementT element({1, 2, 3}); + + buffer_vector v; + v.append(2, element); + + v.push_back(v[0]); + TEST_EQUAL(v.size(), 3, ()); + TEST_EQUAL(v[2], element, ()); +} + UNIT_TEST(BufferVectorBounds) { buffer_vector v; @@ -280,11 +293,14 @@ struct CopyCtorChecker CopyCtorChecker() = default; explicit CopyCtorChecker(char const * s) : m_s(s) {} + CopyCtorChecker(CopyCtorChecker const & rhs) { TEST(rhs.m_s.empty(), ("Copy ctor is called only in resize with default element")); } CopyCtorChecker(CopyCtorChecker && rhs) = default; + + CopyCtorChecker & operator=(CopyCtorChecker &&) = default; CopyCtorChecker & operator=(CopyCtorChecker const &) { TEST(false, ("Assigment operator should not be called")); diff --git a/base/buffer_vector.hpp b/base/buffer_vector.hpp index 65883efef1..4813d71e41 100644 --- a/base/buffer_vector.hpp +++ b/base/buffer_vector.hpp @@ -305,7 +305,7 @@ public: Swap(m_static[i], rhs.m_static[i]); } - void push_back(T const & t) + void push_back(T t) { if (IsDynamic()) { diff --git a/routing/index_graph_starter_joints.hpp b/routing/index_graph_starter_joints.hpp index 7d6ec59299..0a23e437c2 100644 --- a/routing/index_graph_starter_joints.hpp +++ b/routing/index_graph_starter_joints.hpp @@ -494,7 +494,7 @@ bool IndexGraphStarterJoints::FillEdgesAndParentsWeights( if (edges.size() != prevSize) { CHECK_LESS(i, parentWeights.size(), ()); - parentWeights.emplace_back(parentWeights[i]); + parentWeights.push_back(parentWeights[i]); } } } -- 2.45.3 From cea67ce3c031e6969b701f40760eb5ff1c033c91 Mon Sep 17 00:00:00 2001 From: vng Date: Thu, 22 Apr 2021 19:39:37 +0300 Subject: [PATCH 2/3] Cleanup buffer_vector and use range std::move. Signed-off-by: vng --- base/buffer_vector.hpp | 96 +++++++------------------- openlr/graph.cpp | 4 +- openlr/router.cpp | 4 +- routing/index_graph_starter.cpp | 2 +- routing/index_graph_starter_joints.hpp | 4 +- routing/road_graph.cpp | 4 +- routing/transit_world_graph.cpp | 2 +- 7 files changed, 36 insertions(+), 80 deletions(-) diff --git a/base/buffer_vector.hpp b/base/buffer_vector.hpp index 4813d71e41..533ae80ff6 100644 --- a/base/buffer_vector.hpp +++ b/base/buffer_vector.hpp @@ -29,33 +29,10 @@ private: inline bool IsDynamic() const { return m_size == USE_DYNAMIC; } - /// @todo clang on linux doesn't have is_trivially_copyable. -#ifndef OMIM_OS_LINUX - template - std::enable_if_t::value, void> MoveStatic(buffer_vector & rhs) + void MoveStatic(buffer_vector & rhs) { - memcpy(m_static, rhs.m_static, rhs.m_size*sizeof(T)); + std::move(rhs.m_static, rhs.m_static + rhs.m_size, m_static); } - template - std::enable_if_t::value, void> MoveStatic( - buffer_vector & rhs) - { - for (size_t i = 0; i < rhs.m_size; ++i) - Swap(m_static[i], rhs.m_static[i]); - } -#else - template - std::enable_if_t::value, void> MoveStatic(buffer_vector & rhs) - { - memcpy(m_static, rhs.m_static, rhs.m_size*sizeof(T)); - } - template - std::enable_if_t::value, void> MoveStatic(buffer_vector & rhs) - { - for (size_t i = 0; i < rhs.m_size; ++i) - Swap(m_static[i], rhs.m_static[i]); - } -#endif public: typedef T value_type; @@ -115,24 +92,23 @@ public: template void append(TIt beg, TIt end) { - if (IsDynamic()) + if (!IsDynamic()) { - m_dynamic.insert(m_dynamic.end(), beg, end); - return; + size_t const newSize = std::distance(beg, end) + m_size; + if (newSize <= N) + { + while (beg != end) + m_static[m_size++] = *beg++; + return; + } + else + { + m_dynamic.reserve(newSize); + SwitchToDynamic(); + } } - while (beg != end) - { - if (m_size == N) - { - m_dynamic.reserve(N * 2); - SwitchToDynamic(); - while (beg != end) - m_dynamic.push_back(*beg++); - break; - } - m_static[m_size++] = *beg++; - } + m_dynamic.insert(m_dynamic.end(), beg, end); } void append(size_t count, T const & c) @@ -147,7 +123,10 @@ public: return; } else + { + m_dynamic.reserve(newSize); SwitchToDynamic(); + } } m_dynamic.insert(m_dynamic.end(), count, c); @@ -313,36 +292,15 @@ public: return; } - if (m_size < N) - { - m_static[m_size++] = t; - } - else - { - ASSERT_EQUAL(m_size, N, ()); - SwitchToDynamic(); - m_dynamic.push_back(t); - ASSERT_EQUAL(m_dynamic.size(), N + 1, ()); - } - } - - void push_back(T && t) - { - if (IsDynamic()) - { - m_dynamic.push_back(std::move(t)); - return; - } - if (m_size < N) { Swap(m_static[m_size++], t); } else { - ASSERT_EQUAL(m_size, N, ()); + m_dynamic.reserve(N + 1); SwitchToDynamic(); - m_dynamic.push_back(std::move(t)); + m_dynamic.push_back(t); ASSERT_EQUAL(m_dynamic.size(), N + 1, ()); } } @@ -375,7 +333,7 @@ public: } else { - ASSERT_EQUAL(m_size, N, ()); + m_dynamic.reserve(N + 1); SwitchToDynamic(); m_dynamic.emplace_back(std::forward(args)...); ASSERT_EQUAL(m_dynamic.size(), N + 1, ()); @@ -450,12 +408,10 @@ private: { ASSERT_NOT_EQUAL(m_size, static_cast(USE_DYNAMIC), ()); ASSERT_EQUAL(m_dynamic.size(), 0, ()); - m_dynamic.reserve(m_size); - for (size_t i = 0; i < m_size; ++i) - { - m_dynamic.emplace_back(); - Swap(m_static[i], m_dynamic.back()); - } + + m_dynamic.resize(m_size); + std::move(m_static, m_static + m_size, m_dynamic.begin()); + m_size = USE_DYNAMIC; } }; diff --git a/openlr/graph.cpp b/openlr/graph.cpp index 9dd30b4805..fb47060052 100644 --- a/openlr/graph.cpp +++ b/openlr/graph.cpp @@ -28,12 +28,12 @@ void GetRegularEdges(geometry::PointWithAltitude const & junction, IRoadGraph co { auto & es = cache[junction]; (graph.*edgeGetter)(junction, es); - edges.insert(end(edges), begin(es), end(es)); + edges.append(begin(es), end(es)); } else { auto const & es = it->second; - edges.insert(end(edges), begin(es), end(es)); + edges.append(begin(es), end(es)); } } } // namespace diff --git a/openlr/router.cpp b/openlr/router.cpp index 09b46f2dca..4e01d3bb7f 100644 --- a/openlr/router.cpp +++ b/openlr/router.cpp @@ -478,12 +478,12 @@ void Router::GetEdges( { auto & es = cache[u]; (m_graph.*getRegular)(u, es); - edges.insert(edges.end(), es.begin(), es.end()); + edges.append(es.begin(), es.end()); } else { auto const & es = it->second; - edges.insert(edges.end(), es.begin(), es.end()); + edges.append(es.begin(), es.end()); } (m_graph.*getFake)(u, edges); } diff --git a/routing/index_graph_starter.cpp b/routing/index_graph_starter.cpp index 526cefe125..a0fbdb07eb 100644 --- a/routing/index_graph_starter.cpp +++ b/routing/index_graph_starter.cpp @@ -582,7 +582,7 @@ void IndexGraphStarter::AddFakeEdges(Segment const & segment, bool isOutgoing, E } } } - edges.insert(edges.end(), fakeEdges.begin(), fakeEdges.end()); + edges.append(fakeEdges.begin(), fakeEdges.end()); } bool IndexGraphStarter::EndingPassThroughAllowed(Ending const & ending) diff --git a/routing/index_graph_starter_joints.hpp b/routing/index_graph_starter_joints.hpp index 0a23e437c2..75eefe306a 100644 --- a/routing/index_graph_starter_joints.hpp +++ b/routing/index_graph_starter_joints.hpp @@ -456,13 +456,13 @@ bool IndexGraphStarterJoints::FillEdgesAndParentsWeights( // from start to start or end to end vertex. if (vertex == GetStartJoint()) { - edges.insert(edges.end(), m_startOutEdges.begin(), m_startOutEdges.end()); + edges.append(m_startOutEdges.begin(), m_startOutEdges.end()); parentWeights.append(edges.size(), Weight(0.0)); firstFakeId = edges.size(); } else if (vertex == GetFinishJoint()) { - edges.insert(edges.end(), m_endOutEdges.begin(), m_endOutEdges.end()); + edges.append(m_endOutEdges.begin(), m_endOutEdges.end()); // If vertex is FinishJoint, parentWeight is equal to zero, because the first vertex is zero-weight loop. parentWeights.append(edges.size(), Weight(0.0)); } diff --git a/routing/road_graph.cpp b/routing/road_graph.cpp index 27a5f9ce4f..d67e49ee2f 100644 --- a/routing/road_graph.cpp +++ b/routing/road_graph.cpp @@ -226,7 +226,7 @@ void IRoadGraph::GetFakeOutgoingEdges(geometry::PointWithAltitude const & juncti { auto const it = m_fakeOutgoingEdges.find(junction); if (it != m_fakeOutgoingEdges.cend()) - edges.insert(edges.end(), it->second.cbegin(), it->second.cend()); + edges.append(it->second.cbegin(), it->second.cend()); } void IRoadGraph::GetFakeIngoingEdges(geometry::PointWithAltitude const & junction, @@ -234,7 +234,7 @@ void IRoadGraph::GetFakeIngoingEdges(geometry::PointWithAltitude const & junctio { auto const it = m_fakeIngoingEdges.find(junction); if (it != m_fakeIngoingEdges.cend()) - edges.insert(edges.end(), it->second.cbegin(), it->second.cend()); + edges.append(it->second.cbegin(), it->second.cend()); } void IRoadGraph::ResetFakes() diff --git a/routing/transit_world_graph.cpp b/routing/transit_world_graph.cpp index 56a68a3c8b..93b7ed7075 100644 --- a/routing/transit_world_graph.cpp +++ b/routing/transit_world_graph.cpp @@ -66,7 +66,7 @@ void TransitWorldGraph::GetEdgeList(astar::VertexData cons fakeFromReal.emplace_back(s, edge.GetWeight()); } } - edges.insert(edges.end(), fakeFromReal.begin(), fakeFromReal.end()); + edges.append(fakeFromReal.begin(), fakeFromReal.end()); } void TransitWorldGraph::GetEdgeList( -- 2.45.3 From 13fcdd3aba8ba88a5548c05fedd9693264ccf94b Mon Sep 17 00:00:00 2001 From: vng Date: Fri, 23 Apr 2021 18:32:13 +0300 Subject: [PATCH 3/3] Review fixes. Signed-off-by: vng --- base/buffer_vector.hpp | 39 +++++++++++++++------------------------ 1 file changed, 15 insertions(+), 24 deletions(-) diff --git a/base/buffer_vector.hpp b/base/buffer_vector.hpp index 533ae80ff6..a758166924 100644 --- a/base/buffer_vector.hpp +++ b/base/buffer_vector.hpp @@ -97,15 +97,12 @@ public: size_t const newSize = std::distance(beg, end) + m_size; if (newSize <= N) { - while (beg != end) - m_static[m_size++] = *beg++; + std::copy(beg, end, &m_static[m_size]); + m_size = newSize; return; } else - { - m_dynamic.reserve(newSize); - SwitchToDynamic(); - } + SwitchToDynamic(newSize); } m_dynamic.insert(m_dynamic.end(), beg, end); @@ -118,15 +115,12 @@ public: size_t const newSize = count + m_size; if (newSize <= N) { - while (m_size < newSize) - m_static[m_size++] = c; + std::fill_n(&m_static[m_size], count, c); + m_size = newSize; return; } else - { - m_dynamic.reserve(newSize); - SwitchToDynamic(); - } + SwitchToDynamic(newSize); } m_dynamic.insert(m_dynamic.end(), count, c); @@ -165,8 +159,7 @@ public: } else { - m_dynamic.reserve(n); - SwitchToDynamic(); + SwitchToDynamic(n); m_dynamic.resize(n); ASSERT_EQUAL(m_dynamic.size(), n, ()); } @@ -188,9 +181,8 @@ public: } else { - m_dynamic.reserve(n); size_t const oldSize = m_size; - SwitchToDynamic(); + SwitchToDynamic(n); m_dynamic.insert(m_dynamic.end(), n - oldSize, c); ASSERT_EQUAL(m_dynamic.size(), n, ()); } @@ -284,6 +276,7 @@ public: Swap(m_static[i], rhs.m_static[i]); } + /// By value to be consistent with m_vec.push_back(m_vec[0]). void push_back(T t) { if (IsDynamic()) @@ -298,9 +291,8 @@ public: } else { - m_dynamic.reserve(N + 1); - SwitchToDynamic(); - m_dynamic.push_back(t); + SwitchToDynamic(N + 1); + m_dynamic.push_back(std::move(t)); ASSERT_EQUAL(m_dynamic.size(), N + 1, ()); } } @@ -333,8 +325,7 @@ public: } else { - m_dynamic.reserve(N + 1); - SwitchToDynamic(); + SwitchToDynamic(N + 1); m_dynamic.emplace_back(std::forward(args)...); ASSERT_EQUAL(m_dynamic.size(), N + 1, ()); } @@ -368,8 +359,7 @@ public: } else { - m_dynamic.reserve(m_size + n); - SwitchToDynamic(); + SwitchToDynamic(m_size + n); m_dynamic.insert(m_dynamic.begin() + pos, beg, end); } } @@ -404,11 +394,12 @@ public: void erase(iterator it) { erase(it, it + 1); } private: - void SwitchToDynamic() + void SwitchToDynamic(size_t toReserve) { ASSERT_NOT_EQUAL(m_size, static_cast(USE_DYNAMIC), ()); ASSERT_EQUAL(m_dynamic.size(), 0, ()); + m_dynamic.reserve(toReserve); m_dynamic.resize(m_size); std::move(m_static, m_static + m_size, m_dynamic.begin()); -- 2.45.3