From 156038efb2c762eaffb9237a780ef0393e27ceac Mon Sep 17 00:00:00 2001 From: Alexander Shalamov Date: Mon, 4 Mar 2019 15:48:26 +0200 Subject: Review comments: minor changes + formattedSection => formattedSectionID --- include/mbgl/style/expression/expression.hpp | 7 ++++--- include/mbgl/style/expression/formatted.hpp | 2 +- include/mbgl/style/property_expression.hpp | 2 +- src/mbgl/layout/symbol_layout.cpp | 2 +- src/mbgl/renderer/paint_property_binder.hpp | 14 +++++++------- src/mbgl/style/expression/compound_expression.cpp | 4 ++-- src/mbgl/style/expression/is_constant.cpp | 2 +- src/mbgl/text/tagged_string.hpp | 2 +- test/style/property_expression.test.cpp | 6 +++--- 9 files changed, 21 insertions(+), 20 deletions(-) diff --git a/include/mbgl/style/expression/expression.hpp b/include/mbgl/style/expression/expression.hpp index d868b17e00..6276b8dc93 100644 --- a/include/mbgl/style/expression/expression.hpp +++ b/include/mbgl/style/expression/expression.hpp @@ -35,15 +35,16 @@ public: zoom(std::move(zoom_)), feature(feature_), colorRampParameter(std::move(colorRampParameter_)) {} - EvaluationContext& withFormattedSection(const Value* formattedSection_) noexcept { - formattedSection = formattedSection_; + EvaluationContext& withFormattedSectionID(const Value* formattedSectionID_) noexcept { + formattedSectionID = formattedSectionID_; return *this; }; optional zoom; GeometryTileFeature const * feature = nullptr; optional colorRampParameter; - const Value* formattedSection = nullptr; + // Contains formatted section ID (double | std::string) + const Value* formattedSectionID = nullptr; }; template diff --git a/include/mbgl/style/expression/formatted.hpp b/include/mbgl/style/expression/formatted.hpp index 28964de941..02e2f7c3d0 100644 --- a/include/mbgl/style/expression/formatted.hpp +++ b/include/mbgl/style/expression/formatted.hpp @@ -23,7 +23,7 @@ optional toFormattedSectionID(const Variant& variant) { return variant.match( [] (double t) -> FormattedSectionID { return t; }, [] (const std::string& t) -> FormattedSectionID { return t;}, - [] (auto&) -> optional { return nullopt; }); + [] (const auto&) -> optional { return nullopt; }); } struct FormattedSection { diff --git a/include/mbgl/style/property_expression.hpp b/include/mbgl/style/property_expression.hpp index 43e80dfac4..32983e2380 100644 --- a/include/mbgl/style/property_expression.hpp +++ b/include/mbgl/style/property_expression.hpp @@ -34,7 +34,7 @@ template class PropertyExpression final : public PropertyExpressionBase { public: // Second parameter to be used only for conversions from legacy functions. - PropertyExpression(std::unique_ptr expression_, optional defaultValue_ = {}) + PropertyExpression(std::unique_ptr expression_, optional defaultValue_ = nullopt) : PropertyExpressionBase(std::move(expression_)), defaultValue(std::move(defaultValue_)) { } diff --git a/src/mbgl/layout/symbol_layout.cpp b/src/mbgl/layout/symbol_layout.cpp index e95fb56c1d..d6d7eb8c13 100644 --- a/src/mbgl/layout/symbol_layout.cpp +++ b/src/mbgl/layout/symbol_layout.cpp @@ -33,7 +33,7 @@ expression::Value sectionOptionsToValue(const SectionOptions& options) { return (*options.id).match( [] (double t) -> expression::Value { return t; }, [] (const std::string& t) -> expression::Value { return t; }, - [] (auto&) -> expression::Value { return {}; }); + [] (const auto&) -> expression::Value { return {}; }); } return {}; } diff --git a/src/mbgl/renderer/paint_property_binder.hpp b/src/mbgl/renderer/paint_property_binder.hpp index 9659384d33..3b2e253ba1 100644 --- a/src/mbgl/renderer/paint_property_binder.hpp +++ b/src/mbgl/renderer/paint_property_binder.hpp @@ -187,9 +187,9 @@ public: defaultValue(std::move(defaultValue_)) { } void setPatternParameters(const optional&, const optional&, CrossfadeParameters&) override {}; - void populateVertexVector(const GeometryTileFeature& feature, std::size_t length, const ImagePositions&, const optional&, const style::expression::Value& formattedSection) override { + void populateVertexVector(const GeometryTileFeature& feature, std::size_t length, const ImagePositions&, const optional&, const style::expression::Value& formattedSectionID) override { using style::expression::EvaluationContext; - auto evaluated = expression.evaluate(EvaluationContext(&feature).withFormattedSection(&formattedSection), defaultValue); + auto evaluated = expression.evaluate(EvaluationContext(&feature).withFormattedSectionID(&formattedSectionID), defaultValue); this->statistics.add(evaluated); auto value = attributeValue(evaluated); for (std::size_t i = vertexVector.vertexSize(); i < length; ++i) { @@ -243,11 +243,11 @@ public: zoomRange({zoom, zoom + 1}) { } void setPatternParameters(const optional&, const optional&, CrossfadeParameters&) override {}; - void populateVertexVector(const GeometryTileFeature& feature, std::size_t length, const ImagePositions&, const optional&, const style::expression::Value& formattedSection) override { + void populateVertexVector(const GeometryTileFeature& feature, std::size_t length, const ImagePositions&, const optional&, const style::expression::Value& formattedSectionID) override { using style::expression::EvaluationContext; Range range = { - expression.evaluate(EvaluationContext(zoomRange.min, &feature).withFormattedSection(&formattedSection), defaultValue), - expression.evaluate(EvaluationContext(zoomRange.max, &feature).withFormattedSection(&formattedSection), defaultValue), + expression.evaluate(EvaluationContext(zoomRange.min, &feature).withFormattedSectionID(&formattedSectionID), defaultValue), + expression.evaluate(EvaluationContext(zoomRange.max, &feature).withFormattedSectionID(&formattedSectionID), defaultValue), }; this->statistics.add(range.min); this->statistics.add(range.max); @@ -483,9 +483,9 @@ public: PaintPropertyBinders(PaintPropertyBinders&&) = default; PaintPropertyBinders(const PaintPropertyBinders&) = delete; - void populateVertexVectors(const GeometryTileFeature& feature, std::size_t length, const ImagePositions& patternPositions, const optional& patternDependencies, const style::expression::Value& formattedSection = {}) { + void populateVertexVectors(const GeometryTileFeature& feature, std::size_t length, const ImagePositions& patternPositions, const optional& patternDependencies, const style::expression::Value& formattedSectionID = {}) { util::ignore({ - (binders.template get()->populateVertexVector(feature, length, patternPositions, patternDependencies, formattedSection), 0)... + (binders.template get()->populateVertexVector(feature, length, patternPositions, patternDependencies, formattedSectionID), 0)... }); } diff --git a/src/mbgl/style/expression/compound_expression.cpp b/src/mbgl/style/expression/compound_expression.cpp index 27dfe6c151..3dd0ee237b 100644 --- a/src/mbgl/style/expression/compound_expression.cpp +++ b/src/mbgl/style/expression/compound_expression.cpp @@ -664,10 +664,10 @@ const auto& errorCompoundExpression() { const auto& textSectionCompoundExpression() { static auto signature = detail::makeSignature("text-section", [](const EvaluationContext& params) -> Result { - if (!params.formattedSection) { + if (!params.formattedSectionID) { return EvaluationError {"Formatted section is unavailable in the current evaluation context."}; } - return *params.formattedSection; + return *params.formattedSectionID; }); return signature; } diff --git a/src/mbgl/style/expression/is_constant.cpp b/src/mbgl/style/expression/is_constant.cpp index 8ac362373c..25fa4fd4e5 100644 --- a/src/mbgl/style/expression/is_constant.cpp +++ b/src/mbgl/style/expression/is_constant.cpp @@ -24,7 +24,7 @@ bool isFeatureConstant(const Expression& expression) { name == "text-section" ) { return false; - } else if (0 == name.rfind(filter, 0)) { + } else if (0u == name.rfind(filter, 0u)) { // Legacy filters begin with "filter-" and are never constant. return false; } diff --git a/src/mbgl/text/tagged_string.hpp b/src/mbgl/text/tagged_string.hpp index c40c253753..015e263528 100644 --- a/src/mbgl/text/tagged_string.hpp +++ b/src/mbgl/text/tagged_string.hpp @@ -9,7 +9,7 @@ namespace mbgl { using style::expression::FormattedSectionID; struct SectionOptions { - SectionOptions(double scale_, FontStack fontStack_, const optional& id_ = {}) + SectionOptions(double scale_, FontStack fontStack_, const optional& id_ = nullopt) : scale(scale_), fontStackHash(FontStackHasher()(fontStack_)), fontStack(std::move(fontStack_)), id(std::move(id_)) {} diff --git a/test/style/property_expression.test.cpp b/test/style/property_expression.test.cpp index a7d1d25660..a6ee22106a 100644 --- a/test/style/property_expression.test.cpp +++ b/test/style/property_expression.test.cpp @@ -125,13 +125,13 @@ TEST(PropertyExpression, Issue8460) { TEST(PropertyExpression, TextSection) { expression::Value formattedSection1 = 42.0; PropertyExpression ts1(textSection()); - EXPECT_DOUBLE_EQ(42.0, ts1.evaluate(expression::EvaluationContext().withFormattedSection(&formattedSection1))); + EXPECT_DOUBLE_EQ(42.0, ts1.evaluate(expression::EvaluationContext().withFormattedSectionID(&formattedSection1))); expression::Value formattedSection2{"header"s}; PropertyExpression ts2(textSection()); - EXPECT_EQ("header"s, ts2.evaluate(expression::EvaluationContext().withFormattedSection(&formattedSection2))); + EXPECT_EQ("header"s, ts2.evaluate(expression::EvaluationContext().withFormattedSectionID(&formattedSection2))); // Evaluates to default, T(). PropertyExpression ts3(textSection()); - EXPECT_EQ(Color(), ts3.evaluate(expression::EvaluationContext().withFormattedSection(&formattedSection1))); + EXPECT_EQ(Color(), ts3.evaluate(expression::EvaluationContext().withFormattedSectionID(&formattedSection1))); } -- cgit v1.2.1