fix reviewing findingsupstream/clusterProperty_option

3 files changed, 51 insertions, 43 deletions

diff --git a/src/mbgl/style/conversion/geojson_options.cpp b/src/mbgl/style/conversion/geojson_options.cpp
index 7be6ae6e5f..08553e34bb 100644
--- a/src/mbgl/style/conversion/geojson_options.cpp
+++ b/src/mbgl/style/conversion/geojson_options.cpp
@@ -99,6 +99,7 @@ optional<GeoJSONOptions> Converter<GeoJSONOptions>::operator()(const Convertible
             return nullopt;
         }
         GeoJSONOptions::ClusterProperties result;
+        assert(error.message.empty());
         eachMember(
             *clusterProperties,
             [&](const std::string& k,
@@ -107,34 +108,42 @@ optional<GeoJSONOptions> Converter<GeoJSONOptions>::operator()(const Convertible
                 // or ["key" : [[operator, ['accumulated'], ['get', key]], [mapExpression]]]
                 if (!isArray(v) || arrayLength(v) != 2) {
                     error.message =
-                            "GeoJSON source clusterProperties member must be an array with length of 2";
-                    return {};
+                        "GeoJSON source clusterProperties member must be an array with length of 2";
+                    return nullopt;
                 }
                 auto map = expression::dsl::createExpression(arrayMember(v, 1));
+                if (!map) {
+                    error.message =
+                        "Failed to convert GeoJSON source clusterProperties map expression";
+                    return nullopt;
+                }
                 std::unique_ptr<expression::Expression> reduce;
-                if(isArray(arrayMember(v, 0))){
+                if (isArray(arrayMember(v, 0))) {
                     reduce = expression::dsl::createExpression(arrayMember(v, 0));
-                }
-                else{
+                } else {
                     auto reduceOp = toString(arrayMember(v, 0));
                     if (!reduceOp) {
                         error.message =
-                                "GeoJSON source clusterProperties member must contain a valid operator";
-                        return {};
+                            "GeoJSON source clusterProperties member must contain a valid operator";
+                        return nullopt;
                     }
                     std::stringstream ss;
                     // Reformulate reduce expression to [operator, ['accumulated'], ['get', key]]
+                    // The reason to create expression via parsing string instead of invoking function
+                    // createCompoundExpression is due to expression type disunity can’t be resolved
+                    // with current logic of createCompoundExpression
                     ss << std::string(R"([")") << *reduceOp
-                       << std::string(R"(", ["accumulated"], ["get", ")") << k << std::string(R"("]])");
+                       << std::string(R"(", ["accumulated"], ["get", ")") << k
+                       << std::string(R"("]])");
                     reduce = expression::dsl::createExpression(ss.str().c_str());
                 }
-                if (map && reduce) {
-                    result.emplace(k, std::make_pair(std::move(map), std::move(reduce)));
-                } else {
-                    error.message = "Failed to convert GeoJSON source clusterProperties";
-                    return {};
+                if (!reduce) {
+                    error.message =
+                        "Failed to convert GeoJSON source clusterProperties reduce expression";
+                    return nullopt;
                 }
-                return {};
+                result.emplace(k, std::make_pair(std::move(map), std::move(reduce)));
+                return nullopt;
             });
         if (!error.message.empty()) {
             return nullopt;
diff --git a/src/mbgl/style/expression/compound_expression.cpp b/src/mbgl/style/expression/compound_expression.cpp
index 4728e6fd6d..c637856ad9 100644
--- a/src/mbgl/style/expression/compound_expression.cpp
+++ b/src/mbgl/style/expression/compound_expression.cpp
@@ -369,7 +369,7 @@ const auto& lineProgressCompoundExpression() {
 }
 
 const auto& accumulatedCompoundExpression() {
-    static auto signature = detail::makeSignature("accumulated", [](const EvaluationContext& params) -> Result<Value> {
+    const static auto signature = detail::makeSignature("accumulated", [](const EvaluationContext& params) -> Result<Value> {
         if (!params.accumulated) {
             return EvaluationError {
                 "The 'accumulated' expression is unavailable in the current evaluation context."
diff --git a/src/mbgl/style/sources/geojson_source_impl.cpp b/src/mbgl/style/sources/geojson_source_impl.cpp
index 243af8e6f8..c3cb942709 100644
--- a/src/mbgl/style/sources/geojson_source_impl.cpp
+++ b/src/mbgl/style/sources/geojson_source_impl.cpp
@@ -1,6 +1,7 @@
 #include <mbgl/style/sources/geojson_source_impl.hpp>
 #include <mbgl/tile/tile_id.hpp>
 #include <mbgl/util/constants.hpp>
+#include <mbgl/util/feature.hpp>
 #include <mbgl/util/string.hpp>
 
 #include <mapbox/geojsonvt.hpp>
@@ -13,9 +14,9 @@ namespace style {
 
 class GeoJSONVTData : public GeoJSONData {
 public:
-    GeoJSONVTData(const GeoJSON& geoJSON,
-                  const mapbox::geojsonvt::Options& options)
-        : impl(geoJSON, options) {}
+    GeoJSONVTData(const GeoJSON& geoJSON, const mapbox::geojsonvt::Options& options)
+        : impl(geoJSON, options) {
+    }
 
     mapbox::feature::feature_collection<int16_t> getTile(const CanonicalTileID& tileID) final {
         return impl.getTile(tileID.z, tileID.x, tileID.y).features;
@@ -25,9 +26,8 @@ public:
         return {};
     }
 
-    mapbox::feature::feature_collection<double> getLeaves(const std::uint32_t,
-                                                          const std::uint32_t,
-                                                          const std::uint32_t) final {
+    mapbox::feature::feature_collection<double>
+    getLeaves(const std::uint32_t, const std::uint32_t, const std::uint32_t) final {
         return {};
     }
 
@@ -43,7 +43,8 @@ class SuperclusterData : public GeoJSONData {
 public:
     SuperclusterData(const mapbox::feature::feature_collection<double>& features,
                      const mapbox::supercluster::Options& options)
-        : impl(features, options) {}
+        : impl(features, options) {
+    }
 
     mapbox::feature::feature_collection<int16_t> getTile(const CanonicalTileID& tileID) final {
         return impl.getTile(tileID.z, tileID.x, tileID.y);
@@ -68,25 +69,26 @@ private:
 };
 
 template <class T>
-T EvaluateFeature(optional<T> accumulated,
-                  mapbox::feature::feature<double>& f,
-                  const std::shared_ptr<expression::Expression> expression,
-                  T finalDefaultValue = T()) {
+T evaluateFeature(const mapbox::feature::feature<double>& f,
+                  const std::shared_ptr<expression::Expression>& expression,
+                  optional<T> accumulated = nullopt) {
     const expression::EvaluationResult result = expression->evaluate(accumulated, f);
     if (result) {
-        const optional<T> typed = expression::fromExpressionValue<T>(*result);
-        return typed ? *typed : finalDefaultValue;
+        optional<T> typed = expression::fromExpressionValue<T>(*result);
+        if (typed) {
+            return std::move(*typed);
+        }
     }
-    return finalDefaultValue;
+    return T();
 }
 
 GeoJSONSource::Impl::Impl(std::string id_, optional<GeoJSONOptions> options_)
     : Source::Impl(SourceType::GeoJSON, std::move(id_)) {
-        options = options_ ? std::move(*options_) : GeoJSONOptions{};
+    options = options_ ? std::move(*options_) : GeoJSONOptions{};
 }
 
-GeoJSONSource::Impl::Impl(const Impl& other, const GeoJSON& geoJSON) : Source::Impl(other) {
-    options = std::move(other.options);
+GeoJSONSource::Impl::Impl(const Impl& other, const GeoJSON& geoJSON)
+    : Source::Impl(other), options(other.options) {
     constexpr double scale = util::EXTENT / util::tileSize;
     if (options.cluster && geoJSON.is<mapbox::feature::feature_collection<double>>() &&
         !geoJSON.get<mapbox::feature::feature_collection<double>>().empty()) {
@@ -94,26 +96,23 @@ GeoJSONSource::Impl::Impl(const Impl& other, const GeoJSON& geoJSON) : Source::I
         clusterOptions.maxZoom = options.clusterMaxZoom;
         clusterOptions.extent = util::EXTENT;
         clusterOptions.radius = ::round(scale * options.clusterRadius);
-        clusterOptions.map =
-            [&](const mapbox::feature::property_map& properties) -> mapbox::feature::property_map {
-            mapbox::feature::property_map ret{};
+        Feature feature;
+        clusterOptions.map = [&](const PropertyMap& properties) -> PropertyMap {
+            PropertyMap ret{};
             for (const auto& p : options.clusterProperties) {
-                auto feature = mapbox::feature::feature<double>();
                 feature.properties = properties;
-                ret[p.first] = EvaluateFeature<mapbox::feature::value>(nullopt, feature, p.second.first);
+                ret[p.first] = evaluateFeature<Value>(feature, p.second.first);
             }
             return ret;
         };
-        clusterOptions.reduce = [&](mapbox::feature::property_map& toReturn,
-                                    const mapbox::feature::property_map& toFill) {
+        clusterOptions.reduce = [&](PropertyMap& toReturn, const PropertyMap& toFill) {
             for (const auto& p : options.clusterProperties) {
-                if(toFill.count(p.first) == 0){
+                if (toFill.count(p.first) == 0) {
                     continue;
                 }
-                auto feature = mapbox::feature::feature<double>();
                 feature.properties = toFill;
-                optional<mapbox::feature::value> accumulated(toReturn[p.first]);
-                toReturn[p.first] = EvaluateFeature<mapbox::feature::value>(accumulated, feature, p.second.second);    
+                optional<Value> accumulated(toReturn[p.first]);
+                toReturn[p.first] = evaluateFeature<Value>(feature, p.second.second, accumulated);
             }
         };
         data = std::make_shared<SuperclusterData>(