Review comments: minor changes + formattedSection => formattedSectionID

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<float> zoom;
     GeometryTileFeature const * feature = nullptr;
     optional<double> colorRampParameter;
-    const Value* formattedSection = nullptr;
+    // Contains formatted section ID (double | std::string)
+    const Value* formattedSectionID = nullptr;
 };
 
 template <typename T>
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<FormattedSectionID> toFormattedSectionID(const Variant& variant) {
     return variant.match(
             [] (double t) -> FormattedSectionID { return t; },
             [] (const std::string& t) -> FormattedSectionID { return t;},
-            [] (auto&) -> optional<FormattedSectionID> { return nullopt; });
+            [] (const auto&) -> optional<FormattedSectionID> { 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 T>
 class PropertyExpression final : public PropertyExpressionBase {
 public:
     // Second parameter to be used only for conversions from legacy functions.
-    PropertyExpression(std::unique_ptr<expression::Expression> expression_, optional<T> defaultValue_ = {})
+    PropertyExpression(std::unique_ptr<expression::Expression> expression_, optional<T> 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<ImagePosition>&, const optional<ImagePosition>&, CrossfadeParameters&) override {};
-    void populateVertexVector(const GeometryTileFeature& feature, std::size_t length, const ImagePositions&, const optional<PatternDependency>&, const style::expression::Value& formattedSection) override {
+    void populateVertexVector(const GeometryTileFeature& feature, std::size_t length, const ImagePositions&, const optional<PatternDependency>&, 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<ImagePosition>&, const optional<ImagePosition>&, CrossfadeParameters&) override {};
-    void populateVertexVector(const GeometryTileFeature& feature, std::size_t length, const ImagePositions&, const optional<PatternDependency>&, const style::expression::Value& formattedSection) override {
+    void populateVertexVector(const GeometryTileFeature& feature, std::size_t length, const ImagePositions&, const optional<PatternDependency>&, const style::expression::Value& formattedSectionID) override {
         using style::expression::EvaluationContext;
         Range<T> 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<PatternDependency>& patternDependencies, const style::expression::Value& formattedSection = {}) {
+    void populateVertexVectors(const GeometryTileFeature& feature, std::size_t length, const ImagePositions& patternPositions, const optional<PatternDependency>& patternDependencies, const style::expression::Value& formattedSectionID = {}) {
         util::ignore({
-            (binders.template get<Ps>()->populateVertexVector(feature, length, patternPositions, patternDependencies, formattedSection), 0)...
+            (binders.template get<Ps>()->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<Value> {
-        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<FormattedSectionID>& id_ = {})
+    SectionOptions(double scale_, FontStack fontStack_, const optional<FormattedSectionID>& 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<double> 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<std::string> 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<Color> ts3(textSection());
-    EXPECT_EQ(Color(), ts3.evaluate(expression::EvaluationContext().withFormattedSection(&formattedSection1)));
+    EXPECT_EQ(Color(), ts3.evaluate(expression::EvaluationContext().withFormattedSectionID(&formattedSection1)));
 }