[core] GeometryCollection must not be implicitly copied

7 files changed, 44 insertions, 38 deletions

diff --git a/src/mbgl/layout/pattern_layout.hpp b/src/mbgl/layout/pattern_layout.hpp
index 95f8b181fc..858d515347 100644
--- a/src/mbgl/layout/pattern_layout.hpp
+++ b/src/mbgl/layout/pattern_layout.hpp
@@ -106,7 +106,7 @@ public:
             const auto i = patternFeature.i;
             std::unique_ptr<GeometryTileFeature> feature = std::move(patternFeature.feature);
             const PatternLayerMap& patterns = patternFeature.patterns;
-            GeometryCollection geometries = feature->getGeometries();
+            const GeometryCollection& geometries = feature->getGeometries();
 
             bucket->addFeature(*feature, geometries, patternPositions, patterns);
             featureIndex->insert(geometries, i, sourceLayerID, bucketLeaderID);
diff --git a/src/mbgl/layout/symbol_feature.hpp b/src/mbgl/layout/symbol_feature.hpp
index 2c952fc12a..72e88fb55e 100644
--- a/src/mbgl/layout/symbol_feature.hpp
+++ b/src/mbgl/layout/symbol_feature.hpp
@@ -13,7 +13,7 @@ class SymbolFeature : public GeometryTileFeature {
 public:
     SymbolFeature(std::unique_ptr<GeometryTileFeature> feature_) :
         feature(std::move(feature_)),
-        geometry(feature->getGeometries()) // we need a mutable copy of the geometry for mergeLines()
+        geometry(feature->getGeometries().clone()) // we need a mutable copy of the geometry for mergeLines()
     {}
     
     FeatureType getType() const override { return feature->getType(); }
diff --git a/src/mbgl/tile/geometry_tile_data.cpp b/src/mbgl/tile/geometry_tile_data.cpp
index c7aea35944..db9f37ad4b 100644
--- a/src/mbgl/tile/geometry_tile_data.cpp
+++ b/src/mbgl/tile/geometry_tile_data.cpp
@@ -57,32 +57,31 @@ std::vector<GeometryCollection> classifyRings(const GeometryCollection& rings) {
     std::size_t len = rings.size();
 
     if (len <= 1) {
-        polygons.push_back(rings);
+        polygons.emplace_back(rings.clone());
         return polygons;
     }
 
     GeometryCollection polygon;
     int8_t ccw = 0;
 
-    for (std::size_t i = 0; i < len; i++) {
-        double area = signedArea(rings[i]);
-
-        if (area == 0)
-            continue;
+    for (const auto& ring : rings) {
+        double area = signedArea(ring);
+        if (area == 0) continue;
 
-        if (ccw == 0)
+        if (ccw == 0) {
             ccw = (area < 0 ? -1 : 1);
+        }
 
         if (ccw == (area < 0 ? -1 : 1) && !polygon.empty()) {
-            polygons.push_back(polygon);
-            polygon.clear();
+            polygons.emplace_back(std::move(polygon));
         }
 
-        polygon.push_back(rings[i]);
+        polygon.emplace_back(ring);
     }
 
-    if (!polygon.empty())
-        polygons.push_back(polygon);
+    if (!polygon.empty()) {
+        polygons.emplace_back(std::move(polygon));
+    }
 
     return polygons;
 }
diff --git a/src/mbgl/tile/geometry_tile_data.hpp b/src/mbgl/tile/geometry_tile_data.hpp
index a0d41608a3..203afcace7 100644
--- a/src/mbgl/tile/geometry_tile_data.hpp
+++ b/src/mbgl/tile/geometry_tile_data.hpp
@@ -35,6 +35,13 @@ public:
     GeometryCollection(Args&&... args) : std::vector<GeometryCoordinates>(std::forward<Args>(args)...) {}
     GeometryCollection(std::initializer_list<GeometryCoordinates> args)
       : std::vector<GeometryCoordinates>(std::move(args)) {}
+    GeometryCollection(GeometryCollection&&) = default;
+    GeometryCollection& operator=(GeometryCollection&&) = default;
+
+    GeometryCollection clone() const { return GeometryCollection(*this); }
+
+private:
+    GeometryCollection(const GeometryCollection&) = default;
 };
 
 class GeometryTileFeature {
diff --git a/src/mbgl/tile/geometry_tile_worker.cpp b/src/mbgl/tile/geometry_tile_worker.cpp
index a69825d346..f4d57e5bfc 100644
--- a/src/mbgl/tile/geometry_tile_worker.cpp
+++ b/src/mbgl/tile/geometry_tile_worker.cpp
@@ -382,7 +382,7 @@ void GeometryTileWorker::parse() {
                 if (!filter(expression::EvaluationContext { static_cast<float>(this->id.overscaledZ), feature.get() }))
                     continue;
 
-                GeometryCollection geometries = feature->getGeometries();
+                const GeometryCollection& geometries = feature->getGeometries();
                 bucket->addFeature(*feature, geometries, {}, PatternLayerMap ());
                 featureIndex->insert(geometries, i, sourceLayerID, leaderImpl.id);
             }
diff --git a/test/gl/bucket.test.cpp b/test/gl/bucket.test.cpp
index 9c9847cf4d..f97fd53919 100644
--- a/test/gl/bucket.test.cpp
+++ b/test/gl/bucket.test.cpp
@@ -131,7 +131,7 @@ TEST(Buckets, SymbolBucket) {
 
     // SymbolBucket::addFeature() is a no-op.
     GeometryCollection point { { { 0, 0 } } };
-    bucket.addFeature(StubGeometryTileFeature { {}, FeatureType::Point, point, properties }, point, {}, PatternLayerMap());
+    bucket.addFeature(StubGeometryTileFeature { {}, FeatureType::Point, std::move(point), properties }, point, {}, PatternLayerMap());
     ASSERT_FALSE(bucket.hasData());
     ASSERT_FALSE(bucket.needsUpload());
 
diff --git a/test/util/merge_lines.test.cpp b/test/util/merge_lines.test.cpp
index 1c8c7aae8f..5dc846ad6e 100644
--- a/test/util/merge_lines.test.cpp
+++ b/test/util/merge_lines.test.cpp
@@ -42,14 +42,14 @@ TEST(MergeLines, SameText) {
     input1.push_back(SymbolFeatureStub({}, FeatureType::LineString, {{{6, 0}, {7, 0}, {8, 0}}}, properties, aaa, {}, 0));
     input1.push_back(SymbolFeatureStub({}, FeatureType::LineString, {{{5, 0}, {6, 0}}}, properties, aaa, {}, 0));
 
-    const std::vector<StubGeometryTileFeature> expected1 = {
-        { {}, FeatureType::LineString, {{{0, 0}, {1, 0}, {2, 0}, {3, 0}, {4, 0}}}, properties },
-        { {}, FeatureType::LineString, {{{4, 0}, {5, 0}, {6, 0}}}, properties },
-        { {}, FeatureType::LineString, {{{5, 0}, {6, 0}, {7, 0}, {8, 0}, {9, 0}}}, properties },
-        { {}, FeatureType::LineString, { emptyLine }, properties },
-        { {}, FeatureType::LineString, { emptyLine }, properties },
-        { {}, FeatureType::LineString, { emptyLine }, properties }
-    };
+    std::vector<StubGeometryTileFeature> expected1;
+    expected1.emplace_back(StubGeometryTileFeature({}, FeatureType::LineString, {{{0, 0}, {1, 0}, {2, 0}, {3, 0}, {4, 0}}}, properties));
+    expected1.emplace_back(StubGeometryTileFeature({}, FeatureType::LineString, {{{4, 0}, {5, 0}, {6, 0}}}, properties));
+    expected1.emplace_back(StubGeometryTileFeature({}, FeatureType::LineString, {{{5, 0}, {6, 0}, {7, 0}, {8, 0}, {9, 0}}}, properties));
+    expected1.emplace_back(StubGeometryTileFeature({}, FeatureType::LineString, { emptyLine }, properties));
+    expected1.emplace_back(StubGeometryTileFeature({}, FeatureType::LineString, { emptyLine }, properties));
+    expected1.emplace_back(StubGeometryTileFeature({}, FeatureType::LineString, { emptyLine }, properties));
+    
     
     mbgl::util::mergeLines(input1);
 
@@ -65,11 +65,11 @@ TEST(MergeLines, BothEnds) {
     input2.push_back(SymbolFeatureStub { {}, FeatureType::LineString, {{{4, 0}, {5, 0}, {6, 0}}}, properties, aaa, {}, 0 });
     input2.push_back(SymbolFeatureStub { {}, FeatureType::LineString, {{{2, 0}, {3, 0}, {4, 0}}}, properties, aaa, {}, 0 });
 
-    const std::vector<StubGeometryTileFeature> expected2 = {
-        { {}, FeatureType::LineString, {{{0, 0}, {1, 0}, {2, 0}, {3, 0}, {4, 0}, {5, 0}, {6, 0}}}, properties },
-        { {}, FeatureType::LineString, { emptyLine }, properties },
-        { {}, FeatureType::LineString, { emptyLine }, properties }
-    };
+    std::vector<StubGeometryTileFeature> expected2;
+    expected2.emplace_back(StubGeometryTileFeature({}, FeatureType::LineString, {{{0, 0}, {1, 0}, {2, 0}, {3, 0}, {4, 0}, {5, 0}, {6, 0}}}, properties));
+    expected2.emplace_back(StubGeometryTileFeature({}, FeatureType::LineString, { emptyLine }, properties));
+    expected2.emplace_back(StubGeometryTileFeature({}, FeatureType::LineString, { emptyLine }, properties));
+
 
     mbgl::util::mergeLines(input2);
 
@@ -85,11 +85,11 @@ TEST(MergeLines, CircularLines) {
     input3.push_back(SymbolFeatureStub { {}, FeatureType::LineString, {{{2, 0}, {3, 0}, {4, 0}}}, properties, aaa, {}, 0 });
     input3.push_back(SymbolFeatureStub { {}, FeatureType::LineString, {{{4, 0}, {0, 0}}}, properties, aaa, {}, 0 });
 
-    const std::vector<StubGeometryTileFeature> expected3 = {
-        { {}, FeatureType::LineString, {{{0, 0}, {1, 0}, {2, 0}, {3, 0}, {4, 0}, {0, 0}}}, properties },
-        { {}, FeatureType::LineString, { emptyLine }, properties },
-        { {}, FeatureType::LineString, { emptyLine }, properties }
-    };
+    std::vector<StubGeometryTileFeature> expected3;
+    expected3.emplace_back(StubGeometryTileFeature({}, FeatureType::LineString, {{{0, 0}, {1, 0}, {2, 0}, {3, 0}, {4, 0}, {0, 0}}}, properties));
+    expected3.emplace_back(StubGeometryTileFeature({}, FeatureType::LineString, { emptyLine }, properties));
+    expected3.emplace_back(StubGeometryTileFeature({}, FeatureType::LineString, { emptyLine }, properties));
+
 
     mbgl::util::mergeLines(input3);
 
@@ -102,20 +102,20 @@ TEST(MergeLines, EmptyOuterGeometry) {
     std::vector<mbgl::SymbolFeature> input;
     input.push_back(SymbolFeatureStub { {}, FeatureType::LineString, {}, properties, aaa, {}, 0 });
 
-    const std::vector<StubGeometryTileFeature> expected = { { {}, FeatureType::LineString, {}, properties } };
+    const StubGeometryTileFeature expected{ {}, FeatureType::LineString, {}, properties };
 
     mbgl::util::mergeLines(input);
 
-    EXPECT_EQ(input[0].geometry, expected[0].getGeometries());
+    EXPECT_EQ(input[0].geometry, expected.getGeometries());
 }
 
 TEST(MergeLines, EmptyInnerGeometry) {
     std::vector<mbgl::SymbolFeature> input;
     input.push_back(SymbolFeatureStub { {}, FeatureType::LineString, {}, properties, aaa, {}, 0 });
 
-    const std::vector<StubGeometryTileFeature> expected = { { {}, FeatureType::LineString, {}, properties } };
+    const StubGeometryTileFeature expected{ {}, FeatureType::LineString, {}, properties };
 
     mbgl::util::mergeLines(input);
 
-    EXPECT_EQ(input[0].geometry, expected[0].getGeometries());
+    EXPECT_EQ(input[0].geometry, expected.getGeometries());
 }