From 0e0bed8da2a6ceea0a499f5547115653a5d1a833 Mon Sep 17 00:00:00 2001
From: kevin <kevin.li@mapbox.com>
Date: Wed, 12 Feb 2020 17:41:22 +0800
Subject: Fix review comments.

---
 include/mbgl/style/expression/in.hpp | 12 +++-------
 src/mbgl/style/expression/in.cpp     | 45 +++++++++++++++++++++++++-----------
 2 files changed, 34 insertions(+), 23 deletions(-)

diff --git a/include/mbgl/style/expression/in.hpp b/include/mbgl/style/expression/in.hpp
index f3e7551914..a7de17c94d 100644
--- a/include/mbgl/style/expression/in.hpp
+++ b/include/mbgl/style/expression/in.hpp
@@ -17,17 +17,11 @@ public:
     EvaluationResult evaluate(const EvaluationContext& params) const override;
     void eachChild(const std::function<void(const Expression&)>&) const override;
 
-    bool operator==(const Expression& e) const override {
-        if (e.getKind() == Kind::In) {
-            auto rhs = static_cast<const In*>(&e);
-            return *needle == *(rhs->needle) && *haystack == *(rhs->haystack);
-        }
-        return false;
-    }
+    bool operator==(const Expression& e) const override;
 
-    std::vector<optional<Value>> possibleOutputs() const override { return {{true}, {false}}; }
+    std::vector<optional<Value>> possibleOutputs() const override;
 
-    std::string getOperator() const override { return "in"; }
+    std::string getOperator() const override;
 
 private:
     std::unique_ptr<Expression> needle;
diff --git a/src/mbgl/style/expression/in.cpp b/src/mbgl/style/expression/in.cpp
index e0cdc9f260..fa2bd83656 100644
--- a/src/mbgl/style/expression/in.cpp
+++ b/src/mbgl/style/expression/in.cpp
@@ -18,6 +18,10 @@ bool isComparableRuntimeType(type::Type type) {
     return type == type::Boolean || type == type::String || type == type::Number || type == type::Null;
 }
 
+bool isSearchableType(type::Type type) {
+    return type == type::String || type.is<type::Array>() || type == type::Null || type == type::Value;
+}
+
 bool isSearchableRuntimeType(type::Type type) {
     return type == type::String || type.is<type::Array>() || type == type::Null;
 }
@@ -26,7 +30,7 @@ bool isSearchableRuntimeType(type::Type type) {
 In::In(std::unique_ptr<Expression> needle_, std::unique_ptr<Expression> haystack_)
     : Expression(Kind::In, type::Boolean), needle(std::move(needle_)), haystack(std::move(haystack_)) {
     assert(isComparableType(needle->getType()));
-    assert(isSearchableRuntimeType(haystack->getType()) || haystack->getType() == type::Value);
+    assert(isSearchableType(haystack->getType()));
 }
 
 EvaluationResult In::evaluate(const EvaluationContext& params) const {
@@ -58,17 +62,8 @@ EvaluationResult In::evaluate(const EvaluationContext& params) const {
 
     if (evaluatedHaystackType == type::String) {
         const auto haystackString = evaluatedHaystack->get<std::string>();
-        std::string needleValue = "";
-        if (evaluatedNeedleType == type::Boolean) {
-            needleValue = evaluatedNeedle->get<bool>() ? "true" : " false";
-        } else if (evaluatedNeedleType == type::String) {
-            needleValue = evaluatedNeedle->get<std::string>();
-        } else if (evaluatedNeedleType == type::Number) {
-            needleValue = std::to_string(evaluatedNeedle->get<double>());
-            needleValue.erase(needleValue.find_last_not_of('0') + 1, std::string::npos);
-            needleValue.erase(needleValue.find_last_not_of('.') + 1, std::string::npos);
-        }
-        return EvaluationResult(haystackString.find(needleValue) != std::string::npos);
+        const auto needleString = toString(*evaluatedNeedle);
+        return EvaluationResult(haystackString.find(needleString) != std::string::npos);
     } else {
         const auto haystackArray = evaluatedHaystack->get<std::vector<Value>>();
         return EvaluationResult(std::find(haystackArray.begin(), haystackArray.end(), *evaluatedNeedle) !=
@@ -92,9 +87,10 @@ ParseResult In::parse(const Convertible& value, ParsingContext& ctx) {
     }
 
     ParseResult needle = ctx.parse(arrayMember(value, 1), 1, {type::Value});
-    ParseResult haystack = ctx.parse(arrayMember(value, 2), 2, {type::Value});
+    if (!needle) return ParseResult();
 
-    if (!needle || !haystack) return ParseResult();
+    ParseResult haystack = ctx.parse(arrayMember(value, 2), 2, {type::Value});
+    if (!haystack) return ParseResult();
 
     type::Type needleType = (*needle)->getType();
     type::Type haystackType = (*haystack)->getType();
@@ -105,9 +101,30 @@ ParseResult In::parse(const Convertible& value, ParsingContext& ctx) {
         return ParseResult();
     }
 
+    if (!isSearchableType(haystackType)) {
+        ctx.error("Expected second argument to be of type array or string, but found " + toString(haystackType) +
+                  " instead.");
+        return ParseResult();
+    }
     return ParseResult(std::make_unique<In>(std::move(*needle), std::move(*haystack)));
 }
 
+bool In::operator==(const Expression& e) const {
+    if (e.getKind() == Kind::In) {
+        auto rhs = static_cast<const In*>(&e);
+        return *needle == *(rhs->needle) && *haystack == *(rhs->haystack);
+    }
+    return false;
+}
+
+std::vector<optional<Value>> In::possibleOutputs() const {
+    return {{true}, {false}};
+}
+
+std::string In::getOperator() const {
+    return "in";
+}
+
 } // namespace expression
 } // namespace style
 } // namespace mbgl
-- 
cgit v1.2.1