Misc code cleanup

5 files changed, 89 insertions, 108 deletions

diff --git a/include/mbgl/style/filter.hpp b/include/mbgl/style/filter.hpp
index b932e06f51..5034b33bd8 100644
--- a/include/mbgl/style/filter.hpp
+++ b/include/mbgl/style/filter.hpp
@@ -20,7 +20,7 @@ public:
 
     friend bool operator==(const Filter& lhs, const Filter& rhs) {
         if (!lhs.expression || !rhs.expression) {
-            return ((bool) lhs.expression) == ((bool) rhs.expression);
+            return lhs.expression == rhs.expression;
         } else {
             return *(lhs.expression) == *(rhs.expression);
         }
diff --git a/platform/macos/macos.xcworkspace/xcshareddata/IDEWorkspaceChecks.plist b/platform/macos/macos.xcworkspace/xcshareddata/IDEWorkspaceChecks.plist
deleted file mode 100644
index 18d981003d..0000000000
--- a/platform/macos/macos.xcworkspace/xcshareddata/IDEWorkspaceChecks.plist
+++ /dev/null
@@ -1,8 +0,0 @@
-<?xml version="1.0" encoding="UTF-8"?>
-<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
-<plist version="1.0">
-<dict>
-	<key>IDEDidComputeMac32BitWarning</key>
-	<true/>
-</dict>
-</plist>
diff --git a/src/mbgl/style/conversion/filter.cpp b/src/mbgl/style/conversion/filter.cpp
index 7b0540dca2..501556175a 100644
--- a/src/mbgl/style/conversion/filter.cpp
+++ b/src/mbgl/style/conversion/filter.cpp
@@ -10,13 +10,31 @@ namespace mbgl {
 namespace style {
 namespace conversion {
     
-// TODO factor this out
-using namespace mbgl::style::expression;
+static bool isExpression(const Convertible& filter);
+    std::unique_ptr<expression::Expression> convertLegacyFilter(const Convertible& values, Error& error);
 
-using GeometryValue = mapbox::geometry::value;
+optional<Filter> Converter<Filter>::operator()(const Convertible& value, Error& error) const {
+    
+    optional<std::unique_ptr<expression::Expression>> expression;
+    
+    if (isExpression(value)) {
+        expression::ParsingContext parsingContext(expression::type::Boolean);
+        expression::ParseResult parseResult = parsingContext.parseExpression(value);
+        if (!parseResult) {
+            error = { parsingContext.getCombinedErrors() };
+            return {};
+        }
+        expression = std::move(*parseResult);
+    } else {
+        expression = convertLegacyFilter(value, error);
+    }
+    
+    if (!expression) return {};
+    else return { Filter { std::move(*expression) } };
+}
 
 // This is a port from https://github.com/mapbox/mapbox-gl-js/blob/master/src/style-spec/feature_filter/index.js
-static bool isExpressionFilter(const Convertible& filter) {
+static bool isExpression(const Convertible& filter) {
     if (!isArray(filter) || arrayLength(filter) == 0) {
         return false;
     }
@@ -40,7 +58,7 @@ static bool isExpressionFilter(const Convertible& filter) {
     } else if (*op == "any" || *op == "all") {
         for (std::size_t i = 1; i < arrayLength(filter); i++) {
             Convertible f = arrayMember(filter, i);
-            if (!isExpressionFilter(f) && !toBool(f)) {
+            if (!isExpression(f) && !toBool(f)) {
                 return false;
             }
         }
@@ -50,38 +68,19 @@ static bool isExpressionFilter(const Convertible& filter) {
         return true;
     }
 }
-
-static bool isLegacyFilter(const Convertible& filter) {
-    return !isExpressionFilter(filter);
-}
-
-std::unique_ptr<Expression> createLiteralExpression(const Convertible& value) {
-    // TODO handle null case
-    return std::make_unique<Literal>(toExpressionValue(*toValue(value)));
-}
-    
-std::vector<std::unique_ptr<Expression>> createLiteralExpressionArray(const Convertible &input, std::size_t startIndex = 0) {
-    std::vector<std::unique_ptr<Expression>> output;
-    for (std::size_t i = startIndex; i < arrayLength(input); i++) {
-        output.push_back(createLiteralExpression(arrayMember(input, i)));
-    }
-    return output;
-}
-    
-std::unique_ptr<Expression> createExpression(std::string op, std::vector<std::unique_ptr<Expression>> args, Error& error) {
     
+std::unique_ptr<expression::Expression> createExpression(std::string op, std::vector<std::unique_ptr<expression::Expression>> args, Error& error) {
     if (op == "any") {
-        return std::make_unique<Any>(std::move(args));
+        return std::make_unique<expression::Any>(std::move(args));
         
     } else if (op == "all") {
-        return std::make_unique<All>(std::move(args));
+        return std::make_unique<expression::All>(std::move(args));
 
     } else {
-        ParsingContext context;
-        ParseResult parseResult = createCompoundExpression(op, std::move(args), context);
-        
-        if (!parseResult || context.getErrors().size() > 0) {
-            error = { context.getErrors()[0].message };
+        expression::ParsingContext parsingContext(expression::type::Boolean);
+        expression::ParseResult parseResult = createCompoundExpression(op, std::move(args), parsingContext);
+        if (!parseResult) {
+            error = { parsingContext.getCombinedErrors() };
             return {};
         } else {
             return std::move(*parseResult);
@@ -89,40 +88,27 @@ std::unique_ptr<Expression> createExpression(std::string op, std::vector<std::un
     }
 }
 
-std::unique_ptr<Expression> convertLegacyFilter(const Convertible& values, Error& error);
+std::unique_ptr<expression::Expression> createExpression(std::string op, std::unique_ptr<expression::Expression> expression, Error& error) {
+    std::vector<std::unique_ptr<expression::Expression>> args;
+    args.push_back(std::move(expression));
+    return createExpression(op, std::move(args), error);
+}
+    
+std::unique_ptr<expression::Expression> convertLiteral(const Convertible& convertible) {
+    optional<mapbox::geometry::value> value = toValue(convertible);
+    expression::Value expressionValue = value ? expression::toExpressionValue(*value) : expression::Null;
+    return std::make_unique<expression::Literal>(expressionValue);
+}
 
-std::vector<std::unique_ptr<Expression>> createLegacyFilter2Array(const Convertible &input, Error& error, std::size_t startIndex = 0) {
-    std::vector<std::unique_ptr<Expression>> output;
+std::vector<std::unique_ptr<expression::Expression>> convertLiteralArray(const Convertible &input, std::size_t startIndex = 0) {
+    std::vector<std::unique_ptr<expression::Expression>> output;
     for (std::size_t i = startIndex; i < arrayLength(input); i++) {
-        output.push_back(convertLegacyFilter(arrayMember(input, i), error));
+        output.push_back(convertLiteral(arrayMember(input, i)));
     }
     return output;
 }
 
-optional<Filter> Converter<Filter>::operator()(const Convertible& value, Error& error) const {
-    
-    optional<std::unique_ptr<Expression>> expression;
-    
-    if (isLegacyFilter(value)) {
-        expression = convertLegacyFilter(value, error);
-    } else {
-        
-        expression::ParsingContext ctx(expression::type::Boolean);
-        expression::ParseResult parseResult = ctx.parseExpression(value);
-        if (!parseResult) {
-            error = { ctx.getCombinedErrors() };
-            return {};
-        }
-        
-        expression = std::move(*parseResult);
-    }
-    
-    if (!expression) return {};
-    
-    return { Filter { std::move(*expression) } };
-}
-
-    std::unique_ptr<Expression> convertComparisonOp(const Convertible& values, Error& error, optional<std::string> opOverride = {}) {
+std::unique_ptr<expression::Expression> convertLegacyComparisonFilter(const Convertible& values, Error& error, optional<std::string> opOverride = {}) {
     optional<std::string> op = opOverride ? opOverride : toString(arrayMember(values, 0));
     optional<std::string> property = toString(arrayMember(values, 1));
     
@@ -130,83 +116,81 @@ optional<Filter> Converter<Filter>::operator()(const Convertible& value, Error&
         error = { "filter property must be a string" };
         return {};
     } else if (*property == "$type") {
-        return createExpression("filter-type-" + *op, createLiteralExpressionArray(values, 2), error);
+        return createExpression("filter-type-" + *op, convertLiteralArray(values, 2), error);
     } else if (*property == "$id") {
-        return createExpression("filter-id-" + *op, createLiteralExpressionArray(values, 2), error);
+        return createExpression("filter-id-" + *op, convertLiteralArray(values, 2), error);
     } else {
-        return createExpression("filter-" + *op, createLiteralExpressionArray(values, 1), error);
+        return createExpression("filter-" + *op, convertLiteralArray(values, 1), error);
     }
 }
     
-std::unique_ptr<Expression> convertHasOp(std::string property, Error& error) {
+std::unique_ptr<expression::Expression> convertLegacyHasFilter(std::string property, Error& error) {
     if (property == "$type") {
-        return std::make_unique<Literal>(true);
+        return std::make_unique<expression::Literal>(true);
     } else if (property == "$id") {
-        return createExpression("filter-has-id", {}, error);
+        return createExpression("filter-has-id", std::vector<std::unique_ptr<expression::Expression>>(), error);
     } else {
-        std::vector<std::unique_ptr<Expression>> args;
-        args.push_back(std::make_unique<Literal>(property));
-        return createExpression("filter-has", std::move(args), error);
+        return createExpression("filter-has", std::make_unique<expression::Literal>(property), error);
     }
 }
-    
-std::unique_ptr<Expression> convertNegation(std::unique_ptr<Expression> expression, Error& error) {
-    std::vector<std::unique_ptr<Expression>> args;
-    args.push_back(std::move(expression));
-    return createExpression("!", std::move(args), error);
-}
 
-std::unique_ptr<Expression> convertInOp(const Convertible& values, Error& error) {
+std::unique_ptr<expression::Expression> convertLegacyInFilter(const Convertible& values, Error& error) {
     optional<std::string> property = toString(arrayMember(values, 1));
     
     if (!property) {
         error = { "filter property must be a string" };
         return {};
     } else if (arrayLength(values) == 0) {
-        return std::make_unique<Literal>(false);
+        return std::make_unique<expression::Literal>(false);
     } else if (*property == "$type") {
-        return createExpression("filter-type-in", createLiteralExpressionArray(values, 2), error);
+        return createExpression("filter-type-in", convertLiteralArray(values, 2), error);
     } else if (*property == "id") {
-        return createExpression("filter-id-in", createLiteralExpressionArray(values, 2), error);
+        return createExpression("filter-id-in", convertLiteralArray(values, 2), error);
     } else {
-        return createExpression("filter-in", createLiteralExpressionArray(values, 1), error);
+        return createExpression("filter-in", convertLiteralArray(values, 1), error);
     }
 }
+
+std::vector<std::unique_ptr<expression::Expression>> convertLegacyFilterArray(const Convertible &input, Error& error, std::size_t startIndex = 0) {
+    std::vector<std::unique_ptr<expression::Expression>> output;
+    for (std::size_t i = startIndex; i < arrayLength(input); i++) {
+        output.push_back(convertLegacyFilter(arrayMember(input, i), error));
+    }
+    return output;
+}
     
-std::unique_ptr<Expression> convertLegacyFilter(const Convertible& values, Error& error) {
-    
+std::unique_ptr<expression::Expression> convertLegacyFilter(const Convertible& values, Error& error) {
     if (isUndefined(values)) {
-        return std::make_unique<Literal>(true);
+        return std::make_unique<expression::Literal>(true);
     }
     
     optional<std::string> op = toString(arrayMember(values, 0));
-    
+
     if (!op) {
         error = { "filter operator must be a string" };
         return {};
     } else if (arrayLength(values) <= 1) {
-        return std::make_unique<Literal>(*op != "any");
+        return std::make_unique<expression::Literal>(*op != "any");
     } else {
         return (
             *op == "==" ||
             *op == "<" ||
             *op == ">" ||
             *op == "<=" ||
-            *op == ">=" ? convertComparisonOp(values, error) :
-            *op == "!=" ? convertNegation(convertComparisonOp(values, error, {"=="}), error) :
-            *op == "any" ? createExpression("any", createLegacyFilter2Array(values, error, 1), error) :
-            *op == "all" ? createExpression("all", createLegacyFilter2Array(values, error, 1), error) :
-            *op == "none" ? convertNegation(createExpression("any", createLegacyFilter2Array(values, error, 1), error), error) :
-            *op == "in" ? convertInOp(values, error) :
-            *op == "!in" ? convertNegation(convertInOp(values, error), error) :
-            *op == "has" ? convertHasOp(*toString(arrayMember(values, 1)), error) :
-            *op == "!has" ? convertNegation(convertHasOp(*toString(arrayMember(values, 1)), error), error) :
-           std::make_unique<Literal>(true)
+            *op == ">=" ? convertLegacyComparisonFilter(values, error) :
+            *op == "!=" ? createExpression("!", convertLegacyComparisonFilter(values, error, {"=="}), error) :
+            *op == "any" ? createExpression("any", convertLegacyFilterArray(values, error, 1), error) :
+            *op == "all" ? createExpression("all", convertLegacyFilterArray(values, error, 1), error) :
+            *op == "none" ? createExpression("!", createExpression("any", convertLegacyFilterArray(values, error, 1), error), error) :
+            *op == "in" ? convertLegacyInFilter(values, error) :
+            *op == "!in" ? createExpression("!", convertLegacyInFilter(values, error), error) :
+            *op == "has" ? convertLegacyHasFilter(*toString(arrayMember(values, 1)), error) :
+            *op == "!has" ? createExpression("!", convertLegacyHasFilter(*toString(arrayMember(values, 1)), error), error) :
+           std::make_unique<expression::Literal>(true)
         );
     }
 }
-    
+
 } // namespace conversion
 } // namespace style
 } // namespace mbgl
-
diff --git a/src/mbgl/style/filter.cpp b/src/mbgl/style/filter.cpp
index 0da7229e3c..a1c0902898 100644
--- a/src/mbgl/style/filter.cpp
+++ b/src/mbgl/style/filter.cpp
@@ -6,16 +6,15 @@ namespace style {
 
 bool Filter::operator()(const expression::EvaluationContext &context) const {
     
-    // TODO What is our fallback behvaiour?
     if (!this->expression) return true;
     
     const expression::EvaluationResult result = this->expression->evaluate(context);
     if (result) {
         const optional<bool> typed = expression::fromExpressionValue<bool>(*result);
         return typed ? *typed : false;
+    } else {
+        return true;
     }
-    return false;
-    
 }
 
 } // namespace style
diff --git a/test/style/conversion/stringify.test.cpp b/test/style/conversion/stringify.test.cpp
index b8f6872f77..65a665ac90 100644
--- a/test/style/conversion/stringify.test.cpp
+++ b/test/style/conversion/stringify.test.cpp
@@ -74,6 +74,12 @@ TEST(Stringify, Value) {
     ASSERT_EQ(stringify(Value(1.2)), "1.2");
 }
 
+// TODO write new test
+//TEST(Stringify, Filter) {
+//    ASSERT_EQ(stringify(NullFilter()), "null");
+//    ASSERT_EQ(stringify(EqualsFilter { "a", 1.0 }), "[\"==\",\"a\",1.0]");
+//}
+
 TEST(Stringify, CameraFunction) {
     ASSERT_EQ(stringify(CameraFunction<float>(ExponentialStops<float> { {{0, 1}}, 1 })),
         "[\"interpolate\",[\"linear\"],[\"zoom\"],0.0,1.0]");