Adress review: Comments + Tests

4 files changed, 37 insertions, 6 deletions

diff --git a/include/mbgl/style/expression/compound_expression.hpp b/include/mbgl/style/expression/compound_expression.hpp
index a206df1853..431afb4d13 100644
--- a/include/mbgl/style/expression/compound_expression.hpp
+++ b/include/mbgl/style/expression/compound_expression.hpp
@@ -137,18 +137,17 @@ ParseResult parseCompoundExpression(const std::string name, const mbgl::style::c
 ParseResult createCompoundExpression(const CompoundExpressionRegistry::Definition& definition,
                                      std::vector<std::unique_ptr<Expression>> args,
                                      ParsingContext& ctx);
-    
+
 ParseResult createCompoundExpression(const std::string& name,
                                      std::vector<std::unique_ptr<Expression>> args,
                                      ParsingContext& ctx);
-    
+// Convenience method for use expressions that have 0, 1, or 2 args.
 ParseResult createCompoundExpression(const std::string& name, ParsingContext& ctx);
 
 ParseResult createCompoundExpression(const std::string& name,
                                      std::unique_ptr<Expression> arg1,
                                      ParsingContext& ctx);
 
-    
 ParseResult createCompoundExpression(const std::string& name,
                                      std::unique_ptr<Expression> arg1,
                                      std::unique_ptr<Expression> arg2,
diff --git a/src/mbgl/style/conversion/filter.cpp b/src/mbgl/style/conversion/filter.cpp
index 64a0790a70..386d85e921 100644
--- a/src/mbgl/style/conversion/filter.cpp
+++ b/src/mbgl/style/conversion/filter.cpp
@@ -27,7 +27,8 @@ optional<Filter> Converter<Filter>::operator()(const Convertible& value, Error&
         }
     } else {
         std::unique_ptr<Expression> expression = convertLegacyFilter(value, error);
-        return Filter(expression ? optional<std::unique_ptr<Expression>>(std::move(expression)) : optional<std::unique_ptr<Expression>>() );
+        if (!expression) return {};
+        return Filter(optional<std::unique_ptr<Expression>>(std::move(expression)));
     }
 }
 
diff --git a/src/mbgl/style/expression/compound_expression.cpp b/src/mbgl/style/expression/compound_expression.cpp
index e0b10b4882..689f8cdf34 100644
--- a/src/mbgl/style/expression/compound_expression.cpp
+++ b/src/mbgl/style/expression/compound_expression.cpp
@@ -18,7 +18,7 @@ namespace detail {
     The Signature<Fn> structs are wrappers around an "evaluate()" function whose
     purpose is to extract the necessary Type data from the evaluate function's
     type.  There are three key (partial) specializations:
-    
+ 
     Signature<R (Params...)>:
     Wraps a simple evaluate function (const T0&, const T1&, ...) -> Result<U>
  
@@ -31,7 +31,7 @@ namespace detail {
     parameters in addition to its subexpressions, i.e.,
     (const EvaluationParams&, const T0&, const T1&, ...) -> Result<U>.  Needed
     for expressions like ["zoom"], ["get", key], etc.
-    
+ 
     In each of the above evaluate signatures, T0, T1, etc. are the types of
     the successfully evaluated subexpressions.
 */
diff --git a/test/style/filter.test.cpp b/test/style/filter.test.cpp
index 5fd8e234da..e770f707c8 100644
--- a/test/style/filter.test.cpp
+++ b/test/style/filter.test.cpp
@@ -27,6 +27,17 @@ bool filter(const char * json,
     return (*filter)(context);
 }
 
+void invalidFilter(const char * json) {
+    conversion::Error error;
+    optional<Filter> filter = conversion::convertJSON<Filter>(json, error);
+    EXPECT_FALSE(bool(filter));
+    EXPECT_NE(error.message, "");
+}
+
+TEST(Filter, EqualsInvalid) {
+    invalidFilter("[\"==\", \"foo\", null]");
+    invalidFilter("[\"==\", \"foo\", [1, 2]]");
+}
 TEST(Filter, EqualsString) {
     auto f = R"(["==", "foo", "bar"])";
     ASSERT_TRUE(filter(f, {{ "foo", std::string("bar") }}));
@@ -52,6 +63,12 @@ TEST(Filter, EqualsType) {
     auto f = R"(["==", "$type", "LineString"])";
     ASSERT_FALSE(filter(f, {{}}, {}, FeatureType::Point, {}));
     ASSERT_TRUE(filter(f, {{}}, {}, FeatureType::LineString, {}));
+    ASSERT_FALSE(filter(f, {{}}, {}, FeatureType::Point, {}));
+
+    invalidFilter("[\"==\", \"$type\"]");
+    invalidFilter("[\"==\", \"$type\", null]");
+    invalidFilter("[\"==\", \"$type\", \"foo\", 1]");
+    invalidFilter("[\"==\", \"$type\", \"foo\", \"Point\"]");
 }
 
 TEST(Filter, InType) {
@@ -76,6 +93,13 @@ TEST(Filter, Any) {
     ASSERT_TRUE(filter("[\"any\", [\"==\", \"foo\", 0], [\"==\", \"foo\", 1]]", {{ std::string("foo"), int64_t(1) }}));
 }
 
+TEST(Filter, AnyExpression) {
+    ASSERT_FALSE(filter("[\"any\"]"));
+    ASSERT_TRUE(filter("[\"any\", true]"));
+    ASSERT_TRUE(filter("[\"any\",true, false]"));
+    ASSERT_TRUE(filter("[\"any\", true, true]"));
+}
+
 TEST(Filter, All) {
     ASSERT_TRUE(filter("[\"all\"]", {{}}));
     ASSERT_TRUE(filter("[\"all\", [\"==\", \"foo\", 1]]", {{ std::string("foo"), int64_t(1) }}));
@@ -83,6 +107,13 @@ TEST(Filter, All) {
     ASSERT_FALSE(filter("[\"all\", [\"==\", \"foo\", 0], [\"==\", \"foo\", 1]]", {{ std::string("foo"), int64_t(1) }}));
 }
 
+TEST(Filter, AllExpression) {
+    ASSERT_TRUE(filter("[\"all\"]"));
+    ASSERT_TRUE(filter("[\"all\", true]"));
+    ASSERT_FALSE(filter("[\"all\",true, false]"));
+    ASSERT_TRUE(filter("[\"any\", true, true]"));
+}
+
 TEST(Filter, None) {
     ASSERT_TRUE(filter("[\"none\"]"));
     ASSERT_FALSE(filter("[\"none\", [\"==\", \"foo\", 1]]", {{ std::string("foo"), int64_t(1) }}));