[core] Simplify CompoundExpression implementation

6 files changed, 131 insertions, 157 deletions

diff --git a/include/mbgl/style/expression/compound_expression.hpp b/include/mbgl/style/expression/compound_expression.hpp
index c618f2f206..ef10dadb55 100644
--- a/include/mbgl/style/expression/compound_expression.hpp
+++ b/include/mbgl/style/expression/compound_expression.hpp
@@ -5,9 +5,7 @@
 #include <mbgl/style/expression/parsing_context.hpp>
 #include <mbgl/style/expression/type.hpp>
 #include <mbgl/style/expression/value.hpp>
-
 #include <mbgl/util/optional.hpp>
-#include <mbgl/util/variant.hpp>
 
 #include <memory>
 #include <vector>
@@ -16,115 +14,34 @@ namespace mbgl {
 namespace style {
 namespace expression {
 
+namespace detail {
+struct SignatureBase;
+} // namespace detail
+
 /*
     CompoundExpression provides a mechanism for implementing an expression
     simply by providing a list of pure functions of the form
     (const T0& arg0, const T1& arg1, ...) -> Result<U> where T0, T1, ..., U are
     member types of mbgl::style::expression::Value.
- 
+
     The majority of expressions specified in the style-spec are implemented in
     this fashion (see compound_expression.cpp).
 */
-
-
-/*
-    Represents the parameter list for an expression that takes an arbitrary
-    number of arguments (of a specific type).
-*/
-struct VarargsType { type::Type type; };
-template <typename T>
-struct Varargs : std::vector<T> {
-    template <class... Args>
-    Varargs(Args&&... args) : std::vector<T>(std::forward<Args>(args)...) {}
-};
-
-namespace detail {
-// Base class for the Signature<Fn> structs that are used to determine
-// each CompoundExpression definition's type::Type data from the type of its
-// "evaluate" function.
-struct SignatureBase {
-    SignatureBase(type::Type result_, variant<std::vector<type::Type>, VarargsType> params_, std::string name_) :
-        result(std::move(result_)),
-        params(std::move(params_)),
-        name(std::move(name_))
-    {}
-    virtual ~SignatureBase() = default;
-    virtual std::unique_ptr<Expression> makeExpression(std::vector<std::unique_ptr<Expression>>) const = 0;
-    type::Type result;
-    variant<std::vector<type::Type>, VarargsType> params;
-    std::string name;
-};
-} // namespace detail
-
-
-/*
-    Common base class for CompoundExpression<Signature> instances.  Used to
-    allow downcasting (and access to things like name & parameter list) during
-    an Expression tree traversal.
-*/
-class CompoundExpressionBase : public Expression {
+class CompoundExpression : public Expression {
 public:
-    CompoundExpressionBase(std::string name_, const detail::SignatureBase& signature) :
-        Expression(Kind::CompoundExpression, signature.result),
-        name(std::move(name_)),
-        params(signature.params)
-    {}
+    CompoundExpression(const detail::SignatureBase&, std::vector<std::unique_ptr<Expression>>);
 
-    std::string getName() const { return name; }
-    optional<std::size_t> getParameterCount() const {
-        return params.match(
-            [&](const VarargsType&) { return optional<std::size_t>(); },
-            [&](const std::vector<type::Type>& p) -> optional<std::size_t> { return p.size(); }
-        );
-    }
-
-    std::vector<optional<Value>> possibleOutputs() const override {
-        return { nullopt };
-    }
-
-private:
-    std::string name;
-    variant<std::vector<type::Type>, VarargsType> params;
-};
-
-template <typename Signature>
-class CompoundExpression : public CompoundExpressionBase {
-public:
-    using Args = typename Signature::Args;
-    
-    CompoundExpression(const std::string& name_,
-                       Signature signature_,
-                       typename Signature::Args args_) :
-        CompoundExpressionBase(name_, signature_),
-        signature(signature_),
-        args(std::move(args_))
-    {}
-    
-    EvaluationResult evaluate(const EvaluationContext& evaluationParams) const override {
-        return signature.apply(evaluationParams, args);
-    }
-    
-    void eachChild(const std::function<void(const Expression&)>& visit) const override {
-        for (const std::unique_ptr<Expression>& e : args) {
-            visit(*e);
-        }
-    }
+    std::string getOperator() const override;
+    EvaluationResult evaluate(const EvaluationContext& evaluationParams) const override;
+    std::vector<optional<Value>> possibleOutputs() const override;
+    void eachChild(const std::function<void(const Expression&)>& visit) const override;
+    bool operator==(const Expression& e) const override;
 
-    bool operator==(const Expression& e) const override {
-        if (e.getKind() == Kind::CompoundExpression) {
-            auto rhs = static_cast<const CompoundExpression*>(&e);
-            return getName() == rhs->getName() && Expression::childrenEqual(args, rhs->args);
-        }
-        return false;
-    }
-    
-    std::string getOperator() const override {
-        return signature.name;
-    }
+    optional<std::size_t> getParameterCount() const;
 
-private:
-    Signature signature;
-    typename Signature::Args args;
+protected:
+    const detail::SignatureBase& signature;
+    std::vector<std::unique_ptr<Expression>> args;
 };
 
 /*
@@ -136,7 +53,9 @@ struct CompoundExpressionRegistry {
     static std::unordered_map<std::string, Definition> definitions;
 };
 
-ParseResult parseCompoundExpression(const std::string name, const mbgl::style::conversion::Convertible& value, ParsingContext& ctx);
+ParseResult parseCompoundExpression(const std::string name,
+                                    const mbgl::style::conversion::Convertible& value,
+                                    ParsingContext& ctx);
 
 ParseResult createCompoundExpression(const std::string& name,
                                      std::vector<std::unique_ptr<Expression>> args,
diff --git a/include/mbgl/style/expression/is_constant.hpp b/include/mbgl/style/expression/is_constant.hpp
index 065fa30db1..bd758977d8 100644
--- a/include/mbgl/style/expression/is_constant.hpp
+++ b/include/mbgl/style/expression/is_constant.hpp
@@ -10,9 +10,9 @@ namespace expression {
 template <typename T>
 bool isGlobalPropertyConstant(const Expression& expression, const T& properties) {
     if (expression.getKind() == Kind::CompoundExpression) {
-        auto e = static_cast<const CompoundExpressionBase*>(&expression);
+        auto e = static_cast<const CompoundExpression*>(&expression);
         for (const std::string& property : properties) {
-            if (e->getName() == property) {
+            if (e->getOperator() == property) {
                 return false;
             }
         }
diff --git a/src/mbgl/style/expression/compound_expression.cpp b/src/mbgl/style/expression/compound_expression.cpp
index 4a74fa4f76..bbb57e19ab 100644
--- a/src/mbgl/style/expression/compound_expression.cpp
+++ b/src/mbgl/style/expression/compound_expression.cpp
@@ -9,13 +9,50 @@
 #include <mbgl/util/ignore.hpp>
 #include <mbgl/util/string.hpp>
 #include <mbgl/util/platform.hpp>
+#include <mbgl/util/variant.hpp>
+
 #include <cmath>
 
 namespace mbgl {
 namespace style {
 namespace expression {
 
+/*
+    Represents the parameter list for an expression that takes an arbitrary
+    number of arguments (of a specific type).
+*/
+struct VarargsType { type::Type type; };
+
+bool operator==(const VarargsType& lhs, const VarargsType& rhs) {
+    return lhs.type == rhs.type;
+}
+
+template <typename T>
+struct Varargs : std::vector<T> {
+    template <class... Args>
+    Varargs(Args&&... args) : std::vector<T>(std::forward<Args>(args)...) {}
+};
+
 namespace detail {
+// Base class for the Signature<Fn> structs that are used to determine
+// each CompoundExpression definition's type::Type data from the type of its
+// "evaluate" function.
+struct SignatureBase {
+    using Args = std::vector<std::unique_ptr<Expression>>;
+
+    SignatureBase(type::Type result_, variant<std::vector<type::Type>, VarargsType> params_, std::string name_) :
+        result(std::move(result_)),
+        params(std::move(params_)),
+        name(std::move(name_))
+    {}
+    virtual ~SignatureBase() = default;
+
+    virtual EvaluationResult apply(const EvaluationContext&, const Args&) const = 0;
+
+    type::Type result;
+    variant<std::vector<type::Type>, VarargsType> params;
+    std::string name;
+};
 
 /*
     The Signature<Fn> structs are wrappers around an "evaluate()" function whose
@@ -44,8 +81,6 @@ struct Signature;
 // Simple evaluate function (const T0&, const T1&, ...) -> Result<U>
 template <class R, class... Params>
 struct Signature<R (Params...)> : SignatureBase {
-    using Args = std::array<std::unique_ptr<Expression>, sizeof...(Params)>;
-
     Signature(R (*evaluate_)(Params...), std::string name_) :
         SignatureBase(
             valueTypeToExpressionType<std::decay_t<typename R::Value>>(),
@@ -53,26 +88,23 @@ struct Signature<R (Params...)> : SignatureBase {
             std::move(name_)
         ),
         evaluate(evaluate_)    {}
-    
-    EvaluationResult apply(const EvaluationContext& evaluationParameters, const Args& args) const {
-        return applyImpl(evaluationParameters, args, std::index_sequence_for<Params...>{});
-    }
 
-    std::unique_ptr<Expression> makeExpression(std::vector<std::unique_ptr<Expression>> args) const override {
-        typename Signature::Args argsArray;
-        std::copy_n(std::make_move_iterator(args.begin()), sizeof...(Params), argsArray.begin());
-        return std::make_unique<CompoundExpression<Signature>>(name, *this, std::move(argsArray));
+    EvaluationResult apply(const EvaluationContext& evaluationParameters, const Args& args) const override {
+        return applyImpl(evaluationParameters, args, std::index_sequence_for<Params...>{});
     }
 
     R (*evaluate)(Params...);
+
 private:
     template <std::size_t ...I>
     EvaluationResult applyImpl(const EvaluationContext& evaluationParameters, const Args& args, std::index_sequence<I...>) const {
-        const std::array<EvaluationResult, sizeof...(I)> evaluated = {{std::get<I>(args)->evaluate(evaluationParameters)...}};
-        for (const auto& arg : evaluated) {
-            if(!arg) return arg.error();
+        std::array<Value, sizeof...(Params)> evaluated;
+        for (std::size_t i = 0; i < sizeof...(Params); ++i) {
+            const EvaluationResult evaluatedArg = args.at(i)->evaluate(evaluationParameters);
+            if (!evaluatedArg) return evaluatedArg.error();
+            evaluated[i] = std::move(*evaluatedArg);
         }
-        const R value = evaluate(*fromExpressionValue<std::decay_t<Params>>(*(evaluated[I]))...);
+        const R value = evaluate(*fromExpressionValue<std::decay_t<Params>>(evaluated[I])...);
         if (!value) return value.error();
         return *value;
     }
@@ -81,8 +113,6 @@ private:
 // Varargs evaluate function (const Varargs<T>&) -> Result<U>
 template <class R, typename T>
 struct Signature<R (const Varargs<T>&)> : SignatureBase {
-    using Args = std::vector<std::unique_ptr<Expression>>;
-
     Signature(R (*evaluate_)(const Varargs<T>&), std::string name_) :
         SignatureBase(
             valueTypeToExpressionType<std::decay_t<typename R::Value>>(),
@@ -92,11 +122,7 @@ struct Signature<R (const Varargs<T>&)> : SignatureBase {
         evaluate(evaluate_)
     {}
 
-    std::unique_ptr<Expression> makeExpression(std::vector<std::unique_ptr<Expression>> args) const override  {
-        return std::make_unique<CompoundExpression<Signature>>(name, *this, std::move(args));
-    };
-
-    EvaluationResult apply(const EvaluationContext& evaluationParameters, const Args& args) const {
+    EvaluationResult apply(const EvaluationContext& evaluationParameters, const Args& args) const override {
         Varargs<T> evaluated;
         evaluated.reserve(args.size());
         for (const auto& arg : args) {
@@ -108,7 +134,7 @@ struct Signature<R (const Varargs<T>&)> : SignatureBase {
         if (!value) return value.error();
         return *value;
     }
-    
+
     R (*evaluate)(const Varargs<T>&);
 };
 
@@ -116,8 +142,6 @@ struct Signature<R (const Varargs<T>&)> : SignatureBase {
 // (const EvaluationParams&, const T0&, const T1&, ...) -> Result<U>
 template <class R, class... Params>
 struct Signature<R (const EvaluationContext&, Params...)> : SignatureBase {
-    using Args = std::array<std::unique_ptr<Expression>, sizeof...(Params)>;
-
     Signature(R (*evaluate_)(const EvaluationContext&, Params...), std::string name_) :
         SignatureBase(
             valueTypeToExpressionType<std::decay_t<typename R::Value>>(),
@@ -127,29 +151,24 @@ struct Signature<R (const EvaluationContext&, Params...)> : SignatureBase {
         evaluate(evaluate_)
     {}
 
-    std::unique_ptr<Expression> makeExpression(std::vector<std::unique_ptr<Expression>> args) const override {
-        typename Signature::Args argsArray;
-        std::copy_n(std::make_move_iterator(args.begin()), sizeof...(Params), argsArray.begin());
-        return std::make_unique<CompoundExpression<Signature>>(name, *this, std::move(argsArray));
-    }
-
-    EvaluationResult apply(const EvaluationContext& evaluationParameters, const Args& args) const {
+    EvaluationResult apply(const EvaluationContext& evaluationParameters, const Args& args) const override {
         return applyImpl(evaluationParameters, args, std::index_sequence_for<Params...>{});
     }
 
 private:
     template <std::size_t ...I>
     EvaluationResult applyImpl(const EvaluationContext& evaluationParameters, const Args& args, std::index_sequence<I...>) const {
-        const std::array<EvaluationResult, sizeof...(I)> evaluated = {{std::get<I>(args)->evaluate(evaluationParameters)...}};
-        for (const auto& arg : evaluated) {
-            if(!arg) return arg.error();
+        std::array<Value, sizeof...(Params)> evaluated;
+        for (std::size_t i = 0; i < sizeof...(Params); ++i) {
+            const EvaluationResult evaluatedArg = args.at(i)->evaluate(evaluationParameters);
+            if (!evaluatedArg) return evaluatedArg.error();
+            evaluated[i] = std::move(*evaluatedArg);
         }
-        // TODO: assert correct runtime type of each arg value
-        const R value = evaluate(evaluationParameters, *fromExpressionValue<std::decay_t<Params>>(*(evaluated[I]))...);
+        const R value = evaluate(evaluationParameters, *fromExpressionValue<std::decay_t<Params>>(evaluated[I])...);
         if (!value) return value.error();
         return *value;
     }
-    
+
     R (*evaluate)(const EvaluationContext&, Params...);
 };
     
@@ -157,8 +176,6 @@ private:
 // (const EvaluationContext&, const Varargs<T>&) -> Result<U>
 template <class R, typename T>
 struct Signature<R (const EvaluationContext&, const Varargs<T>&)> : SignatureBase {
-    using Args = std::vector<std::unique_ptr<Expression>>;
-    
     Signature(R (*evaluate_)(const EvaluationContext&, const Varargs<T>&), std::string name_) :
     SignatureBase(
                   valueTypeToExpressionType<std::decay_t<typename R::Value>>(),
@@ -167,12 +184,8 @@ struct Signature<R (const EvaluationContext&, const Varargs<T>&)> : SignatureBas
                   ),
     evaluate(evaluate_)
     {}
-    
-    std::unique_ptr<Expression> makeExpression(std::vector<std::unique_ptr<Expression>> args) const override  {
-        return std::make_unique<CompoundExpression<Signature>>(name, *this, std::move(args));
-    };
-    
-    EvaluationResult apply(const EvaluationContext& evaluationParameters, const Args& args) const {
+
+    EvaluationResult apply(const EvaluationContext& evaluationParameters, const Args& args) const override {
         Varargs<T> evaluated;
         evaluated.reserve(args.size());
         for (const auto& arg : args) {
@@ -184,7 +197,7 @@ struct Signature<R (const EvaluationContext&, const Varargs<T>&)> : SignatureBas
         if (!value) return value.error();
         return *value;
     }
-    
+
     R (*evaluate)(const EvaluationContext&, const Varargs<T>&);
 };
 
@@ -680,7 +693,7 @@ static ParseResult createCompoundExpression(const Definition& definition,
         }
 
         if (signatureContext.getErrors().size() == 0) {
-            return ParseResult(signature->makeExpression(std::move(args)));
+            return ParseResult(std::make_unique<CompoundExpression>(*signature, std::move(args)));
         }
     }
 
@@ -792,6 +805,48 @@ ParseResult createCompoundExpression(const std::string& name,
     return createCompoundExpression(CompoundExpressionRegistry::definitions.at(name), std::move(args), ctx);
 }
 
+CompoundExpression::CompoundExpression(const detail::SignatureBase& signature_, std::vector<std::unique_ptr<Expression>> args_) :
+    Expression(Kind::CompoundExpression, signature_.result),
+    signature(signature_),
+    args(std::move(args_))
+{}
+
+std::string CompoundExpression::getOperator() const {
+    return signature.name;
+}
+
+EvaluationResult CompoundExpression::evaluate(const EvaluationContext& evaluationParams) const {
+    return signature.apply(evaluationParams, args);
+}
+
+optional<std::size_t> CompoundExpression::getParameterCount() const {
+    return signature.params.match(
+        [&](const VarargsType&) { return optional<std::size_t>(); },
+        [&](const std::vector<type::Type>& p) -> optional<std::size_t> { return p.size(); }
+    );
+}
+
+std::vector<optional<Value>> CompoundExpression::possibleOutputs() const {
+    return { nullopt };
+}
+
+void CompoundExpression::eachChild(const std::function<void(const Expression&)>& visit) const {
+    for (const std::unique_ptr<Expression>& e : args) {
+        visit(*e);
+    }
+}
+
+bool CompoundExpression::operator==(const Expression& e) const {
+    if (e.getKind() == Kind::CompoundExpression) {
+        auto rhs = static_cast<const CompoundExpression*>(&e);
+        return signature.name == rhs->signature.name &&
+            signature.result == rhs->signature.result &&
+            signature.params == rhs->signature.params &&
+            Expression::childrenEqual(args, rhs->args);
+    }
+    return false;
+}
+
 } // namespace expression
 } // namespace style
 } // namespace mbgl
diff --git a/src/mbgl/style/expression/find_zoom_curve.cpp b/src/mbgl/style/expression/find_zoom_curve.cpp
index a27f8560ef..457ae8c927 100644
--- a/src/mbgl/style/expression/find_zoom_curve.cpp
+++ b/src/mbgl/style/expression/find_zoom_curve.cpp
@@ -34,8 +34,8 @@ optional<variant<const Interpolate*, const Step*, ParsingError>> findZoomCurve(c
     case Kind::Interpolate: {
         auto curve = static_cast<const Interpolate*>(e);
         if (curve->getInput()->getKind() == Kind::CompoundExpression) {
-            auto z = static_cast<CompoundExpressionBase*>(curve->getInput().get());
-            if (z && z->getName() == "zoom") {
+            auto z = static_cast<CompoundExpression*>(curve->getInput().get());
+            if (z && z->getOperator() == "zoom") {
                 result = {curve};
             }
         }
@@ -44,8 +44,8 @@ optional<variant<const Interpolate*, const Step*, ParsingError>> findZoomCurve(c
     case Kind::Step: {
         auto step = static_cast<const Step*>(e);
         if (step->getInput()->getKind() == Kind::CompoundExpression) {
-            auto z = static_cast<CompoundExpressionBase*>(step->getInput().get());
-            if (z && z->getName() == "zoom") {
+            auto z = static_cast<CompoundExpression*>(step->getInput().get());
+            if (z && z->getOperator() == "zoom") {
                 result = {step};
             }
         }
diff --git a/src/mbgl/style/expression/is_constant.cpp b/src/mbgl/style/expression/is_constant.cpp
index 3b1f1aba8c..69f27f6fef 100644
--- a/src/mbgl/style/expression/is_constant.cpp
+++ b/src/mbgl/style/expression/is_constant.cpp
@@ -10,8 +10,8 @@ constexpr static const char filter[] = "filter-";
 
 bool isFeatureConstant(const Expression& expression) {
     if (expression.getKind() == Kind::CompoundExpression) {
-        auto e = static_cast<const CompoundExpressionBase*>(&expression);
-        const std::string name = e->getName();
+        auto e = static_cast<const CompoundExpression*>(&expression);
+        const std::string name = e->getOperator();
         optional<std::size_t> parameterCount = e->getParameterCount();
         if (name == "get" && parameterCount && *parameterCount == 1) {
             return false;
diff --git a/src/mbgl/style/expression/parsing_context.cpp b/src/mbgl/style/expression/parsing_context.cpp
index a4c04b03b1..979eb58236 100644
--- a/src/mbgl/style/expression/parsing_context.cpp
+++ b/src/mbgl/style/expression/parsing_context.cpp
@@ -38,8 +38,8 @@ bool isConstant(const Expression& expression) {
     }
 
     if (expression.getKind() == Kind::CompoundExpression) {
-        auto compound = static_cast<const CompoundExpressionBase*>(&expression);
-        if (compound->getName() == "error") {
+        auto compound = static_cast<const CompoundExpression*>(&expression);
+        if (compound->getOperator() == "error") {
             return false;
         }
     }