From d73d57233a8f256e7fd1a588ab398676034d893c Mon Sep 17 00:00:00 2001 From: zmiao Date: Wed, 14 Aug 2019 14:20:05 +0300 Subject: fix reviewing findings --- src/mbgl/style/conversion/geojson_options.cpp | 37 +++++++++------ src/mbgl/style/expression/compound_expression.cpp | 2 +- src/mbgl/style/sources/geojson_source_impl.cpp | 55 +++++++++++------------ 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 Converter::operator()(const Convertible return nullopt; } GeoJSONOptions::ClusterProperties result; + assert(error.message.empty()); eachMember( *clusterProperties, [&](const std::string& k, @@ -107,34 +108,42 @@ optional Converter::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 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 { + const static auto signature = detail::makeSignature("accumulated", [](const EvaluationContext& params) -> Result { 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 #include #include +#include #include #include @@ -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 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 getLeaves(const std::uint32_t, - const std::uint32_t, - const std::uint32_t) final { + mapbox::feature::feature_collection + 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& features, const mapbox::supercluster::Options& options) - : impl(features, options) {} + : impl(features, options) { + } mapbox::feature::feature_collection getTile(const CanonicalTileID& tileID) final { return impl.getTile(tileID.z, tileID.x, tileID.y); @@ -68,25 +69,26 @@ private: }; template -T EvaluateFeature(optional accumulated, - mapbox::feature::feature& f, - const std::shared_ptr expression, - T finalDefaultValue = T()) { +T evaluateFeature(const mapbox::feature::feature& f, + const std::shared_ptr& expression, + optional accumulated = nullopt) { const expression::EvaluationResult result = expression->evaluate(accumulated, f); if (result) { - const optional typed = expression::fromExpressionValue(*result); - return typed ? *typed : finalDefaultValue; + optional typed = expression::fromExpressionValue(*result); + if (typed) { + return std::move(*typed); + } } - return finalDefaultValue; + return T(); } GeoJSONSource::Impl::Impl(std::string id_, optional 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>() && !geoJSON.get>().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(); feature.properties = properties; - ret[p.first] = EvaluateFeature(nullopt, feature, p.second.first); + ret[p.first] = evaluateFeature(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(); feature.properties = toFill; - optional accumulated(toReturn[p.first]); - toReturn[p.first] = EvaluateFeature(accumulated, feature, p.second.second); + optional accumulated(toReturn[p.first]); + toReturn[p.first] = evaluateFeature(feature, p.second.second, accumulated); } }; data = std::make_shared( -- cgit v1.2.1