Address review findings/nit

mapbox · Feb 11, 2020 · a868c3c · a868c3c
1 parent 08a64c2
commit a868c3c
Show file tree

Hide file tree

Showing 3 changed files with 13 additions and 15 deletions.
diff --git a/expression-test/expression_test_parser.cpp b/expression-test/expression_test_parser.cpp
@@ -259,14 +259,13 @@ bool parseInputs(const JSValue& inputsValue, TestData& data) {
         // Parse canonicalID
         optional<CanonicalTileID> canonical;
         if (evaluationContext.HasMember("canonicalID")) {
-            assert(evaluationContext["canonicalID"].IsObject());
-            assert(
-                evaluationContext["canonicalID"].HasMember("z") && evaluationContext["canonicalID"]["z"].IsNumber() &&
-                evaluationContext["canonicalID"].HasMember("x") && evaluationContext["canonicalID"]["x"].IsNumber() &&
-                evaluationContext["canonicalID"].HasMember("y") && evaluationContext["canonicalID"]["y"].IsNumber());
-            canonical = CanonicalTileID(evaluationContext["canonicalID"]["z"].GetUint(),
-                                        evaluationContext["canonicalID"]["x"].GetUint(),
-                                        evaluationContext["canonicalID"]["y"].GetUint());
+            const auto& canonicalIDObject = evaluationContext["canonicalID"];
+            assert(canonicalIDObject.IsObject());
+            assert(canonicalIDObject.HasMember("z") && canonicalIDObject["z"].IsNumber());
+            assert(canonicalIDObject.HasMember("x") && canonicalIDObject["x"].IsNumber());
+            assert(canonicalIDObject.HasMember("y") && canonicalIDObject["y"].IsNumber());
+            canonical = CanonicalTileID(
+                canonicalIDObject["z"].GetUint(), canonicalIDObject["x"].GetUint(), canonicalIDObject["y"].GetUint());
         }
 
         // Parse availableImages

diff --git a/include/mbgl/style/expression/within.hpp b/include/mbgl/style/expression/within.hpp
@@ -15,7 +15,7 @@ class Within final : public Expression {
 public:
     explicit Within(GeoJSON geojson);
 
-    ~Within() final;
+    ~Within() override;
 
     EvaluationResult evaluate(const EvaluationContext&) const override;
 

diff --git a/src/mbgl/style/expression/within.cpp b/src/mbgl/style/expression/within.cpp
@@ -44,7 +44,7 @@ bool pointWithinPolygon(const mbgl::Point<double>& point, const mapbox::geometry
     for (auto ring : polygon) {
         auto size = ring.size();
         // loop through every edge of the ring
-        for (decltype(size) i = 0; i < size - 1; ++i) {
+        for (std::size_t i = 0; i < size - 1; ++i) {
             if (rayIntersect(point, ring[i], ring[i + 1])) {
                 within = !within;
             }
@@ -55,8 +55,7 @@ bool pointWithinPolygon(const mbgl::Point<double>& point, const mapbox::geometry
 
 bool pointWithinPolygons(const mbgl::Point<double>& point, const mapbox::geometry::multi_polygon<double>& polygons) {
     for (auto polygon : polygons) {
-        auto within = pointWithinPolygon(point, polygon);
-        if (within) return true;
+        if (pointWithinPolygon(point, polygon)) return true;
     }
     return false;
 }
@@ -128,7 +127,7 @@ namespace expression {
 
 Within::Within(GeoJSON geojson) : Expression(Kind::Within, type::Boolean), geoJSONSource(std::move(geojson)) {}
 
-Within::~Within() {}
+Within::~Within() = default;
 
 using namespace mbgl::style::conversion;
 
@@ -140,9 +139,9 @@ EvaluationResult Within::evaluate(const EvaluationContext& params) const {
     // Currently only support Point/Points in Polygon/Polygons
     if (geometryType == FeatureType::Point) {
         return pointsWithinPolygons(*params.feature, *params.canonical, geoJSONSource);
-    } else {
-        mbgl::Log::Warning(mbgl::Event::General, "within expression currently only support 'Point' geometry type");
     }
+    mbgl::Log::Warning(mbgl::Event::General, "within expression currently only support 'Point' geometry type");
+
     return false;
 }