Address some review comments:

 - Replace null unique_ptr<Expression> with optional<unique_ptr>
 - Use make_shared instead of shared_ptr(make_unique)
 - Accept awkward dead code paths for compiler's sake
 - Remove non-functional "serialize" for Collator.

11 files changed, 47 insertions, 67 deletions

diff --git a/include/mbgl/style/expression/collator.hpp b/include/mbgl/style/expression/collator.hpp
index 499e205320..f802554e43 100644
--- a/include/mbgl/style/expression/collator.hpp
+++ b/include/mbgl/style/expression/collator.hpp
@@ -19,11 +19,6 @@ public:
     int compare(const std::string& lhs, const std::string& rhs) const;
 
     std::string resolvedLocale() const;
-
-    // TODO: This serialization shouldn't ever be used, but since we're part of
-    // mbgl::style::expression::Value we're expected to have a serialize()
-    mbgl::Value serialize() const;
-
 private:
     class Impl;
     // TODO: Figure out right copy semantics for Collator -- sharing an underlying implementation
diff --git a/include/mbgl/style/expression/collator_expression.hpp b/include/mbgl/style/expression/collator_expression.hpp
index 9e7b527e89..2551cd19c8 100644
--- a/include/mbgl/style/expression/collator_expression.hpp
+++ b/include/mbgl/style/expression/collator_expression.hpp
@@ -14,7 +14,7 @@ class CollatorExpression : public Expression {
 public:
     CollatorExpression(std::unique_ptr<Expression> caseSensitive,
              std::unique_ptr<Expression> diacriticSensitive,
-             std::unique_ptr<Expression> locale);
+             optional<std::unique_ptr<Expression>> locale);
 
     EvaluationResult evaluate(const EvaluationContext&) const override;
     static ParseResult parse(const mbgl::style::conversion::Convertible&, ParsingContext&);
@@ -36,7 +36,7 @@ public:
 private:
     std::unique_ptr<Expression> caseSensitive;
     std::unique_ptr<Expression> diacriticSensitive;
-    std::unique_ptr<Expression> locale;
+    optional<std::unique_ptr<Expression>> locale;
 };
 
 } // namespace expression
diff --git a/include/mbgl/style/expression/equals.hpp b/include/mbgl/style/expression/equals.hpp
index 3458eede91..1e8bf7acef 100644
--- a/include/mbgl/style/expression/equals.hpp
+++ b/include/mbgl/style/expression/equals.hpp
@@ -13,7 +13,7 @@ namespace expression {
 
 class Equals : public Expression {
 public:
-    Equals(std::unique_ptr<Expression> lhs, std::unique_ptr<Expression> rhs, std::unique_ptr<Expression> collator, bool negate);
+    Equals(std::unique_ptr<Expression> lhs, std::unique_ptr<Expression> rhs, optional<std::unique_ptr<Expression>> collator, bool negate);
 
     static ParseResult parse(const mbgl::style::conversion::Convertible&, ParsingContext&);
 
@@ -26,7 +26,7 @@ public:
 private:
     std::unique_ptr<Expression> lhs;
     std::unique_ptr<Expression> rhs;
-    std::unique_ptr<Expression> collator;
+    optional<std::unique_ptr<Expression>> collator;
     bool negate;
 };
 
diff --git a/include/mbgl/style/expression/type.hpp b/include/mbgl/style/expression/type.hpp
index 0909b67c81..5c19334a45 100644
--- a/include/mbgl/style/expression/type.hpp
+++ b/include/mbgl/style/expression/type.hpp
@@ -62,7 +62,7 @@ struct ValueType {
 };
     
 struct CollatorType {
-    constexpr CollatorType() {}; // NOLINT
+    constexpr CollatorType() {};
     std::string getName() const { return "collator"; }
     bool operator==(const CollatorType&) const { return true; }
 };
@@ -74,8 +74,8 @@ constexpr BooleanType Boolean;
 constexpr ColorType Color;
 constexpr ValueType Value;
 constexpr ObjectType Object;
-constexpr ErrorType Error;
 constexpr CollatorType Collator;
+constexpr ErrorType Error;
 
 struct Array;
 
@@ -88,8 +88,8 @@ using Type = variant<
     ObjectType,
     ValueType,
     mapbox::util::recursive_wrapper<Array>,
-    ErrorType,
-    CollatorType>;
+    CollatorType,
+    ErrorType>;
 
 struct Array {
     explicit Array(Type itemType_) : itemType(std::move(itemType_)) {}
diff --git a/platform/android/src/text/collator.cpp b/platform/android/src/text/collator.cpp
index 0ef5f45ad0..37af0a1c5b 100644
--- a/platform/android/src/text/collator.cpp
+++ b/platform/android/src/text/collator.cpp
@@ -179,7 +179,7 @@ private:
 
 
 Collator::Collator(bool caseSensitive, bool diacriticSensitive, optional<std::string> locale_)
-    : impl(std::make_unique<Impl>(caseSensitive, diacriticSensitive, std::move(locale_)))
+    : impl(std::make_shared<Impl>(caseSensitive, diacriticSensitive, std::move(locale_)))
 {}
 
 bool Collator::operator==(const Collator& other) const {
@@ -194,11 +194,6 @@ std::string Collator::resolvedLocale() const {
     return impl->resolvedLocale();
 }
 
-mbgl::Value Collator::serialize() const {
-    return mbgl::Value(true);
-}
-
-
 } // namespace expression
 } // namespace style
 } // namespace mbgl
diff --git a/platform/darwin/src/collator.mm b/platform/darwin/src/collator.mm
index 0d17d0885f..8d9693abbf 100644
--- a/platform/darwin/src/collator.mm
+++ b/platform/darwin/src/collator.mm
@@ -6,7 +6,7 @@
 #import <Foundation/Foundation.h>
 
 namespace mbgl {
-    
+
 std::string localeIdentifier(const std::string& bcp47) {
     // Based on Apple docs at:
     // https://developer.apple.com/library/content/documentation/MacOSX/Conceptual/BPInternational/LanguageandLocaleIDs/LanguageandLocaleIDs.html#//apple_ref/doc/uid/10000171i-CH15-SW9
@@ -17,16 +17,16 @@ std::string localeIdentifier(const std::string& bcp47) {
     } else {
         localeIdentifier << *(languageTag.language);
     }
-    
+
     if (languageTag.script) {
         localeIdentifier << "-" << *(languageTag.script);
     }
-    
+
     if (languageTag.region) {
         // TODO: Does locale identifier support UN M.49? If not we need to do a conversion or document
         localeIdentifier << "_" << *(languageTag.region);
     }
-    
+
     return localeIdentifier.str();
 }
 
@@ -42,12 +42,12 @@ public:
                 [[NSLocale alloc] initWithLocaleIdentifier:@((*locale_).c_str())] :
                 [NSLocale currentLocale])
     {}
-    
+
     bool operator==(const Impl& other) const {
         return options == other.options &&
             [[locale localeIdentifier] isEqualToString:[other.locale localeIdentifier]];
     }
-    
+
     int compare(const std::string& lhs, const std::string& rhs) const {
         NSString* nsLhs = @(lhs.c_str());
         NSString* nsRhs = @(rhs.c_str());
@@ -55,10 +55,10 @@ public:
         // experimentally we've checked that if LHS is a prefix of RHS compare returns -1
         // https://developer.apple.com/documentation/foundation/nsstring/1414561-compare
         NSRange compareRange = NSMakeRange(0, nsLhs.length);
-        
+
         return [nsLhs compare:nsRhs options:options range:compareRange locale:locale];
     }
-    
+
     std::string resolvedLocale() const {
         return [locale localeIdentifier].UTF8String;
     }
@@ -69,7 +69,7 @@ private:
 
 
 Collator::Collator(bool caseSensitive, bool diacriticSensitive, optional<std::string> locale_)
-    : impl(std::make_unique<Impl>(caseSensitive, diacriticSensitive, std::move(locale_)))
+    : impl(std::make_shared<Impl>(caseSensitive, diacriticSensitive, std::move(locale_)))
 {}
 
 bool Collator::operator==(const Collator& other) const {
@@ -83,10 +83,6 @@ int Collator::compare(const std::string& lhs, const std::string& rhs) const {
 std::string Collator::resolvedLocale() const {
     return impl->resolvedLocale();
 }
-mbgl::Value Collator::serialize() const {
-    return mbgl::Value(true);
-}
-
 
 } // namespace expression
 } // namespace style
diff --git a/platform/default/collator.cpp b/platform/default/collator.cpp
index afd905d860..0379b614c9 100644
--- a/platform/default/collator.cpp
+++ b/platform/default/collator.cpp
@@ -58,7 +58,7 @@ private:
 
 
 Collator::Collator(bool caseSensitive, bool diacriticSensitive, optional<std::string> locale_)
-    : impl(std::make_unique<Impl>(caseSensitive, diacriticSensitive, std::move(locale_)))
+    : impl(std::make_shared<Impl>(caseSensitive, diacriticSensitive, std::move(locale_)))
 {}
 
 bool Collator::operator==(const Collator& other) const {
@@ -73,10 +73,6 @@ std::string Collator::resolvedLocale() const {
     return impl->resolvedLocale();
 }
 
-mbgl::Value Collator::serialize() const {
-    return mbgl::Value(true);
-}
-
 
 } // namespace expression
 } // namespace style
diff --git a/platform/node/src/node_expression.cpp b/platform/node/src/node_expression.cpp
index 165b875d97..c0b512a899 100644
--- a/platform/node/src/node_expression.cpp
+++ b/platform/node/src/node_expression.cpp
@@ -145,13 +145,13 @@ struct ToValue {
     }
 
     v8::Local<v8::Value> operator()(const Collator&) {
-        // TODO: Collators are excluded from constant folding and there's no Literal parser
+        // Collators are excluded from constant folding and there's no Literal parser
         // for them so there shouldn't be any way to serialize this value.
-        // Is there a better way to omit this?
+        assert(false);
         Nan::EscapableHandleScope scope;
         return scope.Escape(Nan::Null());
     }
-    
+
     v8::Local<v8::Value> operator()(const mbgl::Color& color) {
         return operator()(std::vector<Value> {
             static_cast<double>(color.r),
diff --git a/src/mbgl/style/expression/collator_expression.cpp b/src/mbgl/style/expression/collator_expression.cpp
index 0863807296..f5e4e3fdff 100644
--- a/src/mbgl/style/expression/collator_expression.cpp
+++ b/src/mbgl/style/expression/collator_expression.cpp
@@ -9,7 +9,7 @@ namespace expression {
     
 CollatorExpression::CollatorExpression(std::unique_ptr<Expression> caseSensitive_,
                    std::unique_ptr<Expression> diacriticSensitive_,
-                   std::unique_ptr<Expression> locale_)
+                   optional<std::unique_ptr<Expression>> locale_)
     : Expression(type::Collator)
     , caseSensitive(std::move(caseSensitive_))
     , diacriticSensitive(std::move(diacriticSensitive_))
@@ -61,25 +61,21 @@ ParseResult CollatorExpression::parse(const Convertible& value, ParsingContext&
         }
     }
     
-    std::unique_ptr<Expression> localePtr;
-    if (locale) {
-        localePtr = std::move(*locale);
-    }
-    return ParseResult(std::make_unique<CollatorExpression>(std::move(*caseSensitive), std::move(*diacriticSensitive), std::move(localePtr)));
+    return ParseResult(std::make_unique<CollatorExpression>(std::move(*caseSensitive), std::move(*diacriticSensitive), std::move(locale)));
 }
     
 void CollatorExpression::eachChild(const std::function<void(const Expression&)>& fn) const {
     fn(*caseSensitive);
     fn(*diacriticSensitive);
-    if (locale.get()) {
-        fn(*locale);
+    if (locale) {
+        fn(**locale);
     }
 }
     
 bool CollatorExpression::operator==(const Expression& e) const {
     if (auto rhs = dynamic_cast<const CollatorExpression*>(&e)) {
-        if ((locale.get() && (!rhs->locale.get() || *locale != *(rhs->locale))) ||
-            (!locale.get() && rhs->locale.get())) {
+        if ((locale && (!rhs->locale || **locale != **(rhs->locale))) ||
+            (!locale && rhs->locale)) {
             return false;
         }
         return *caseSensitive == *(rhs->caseSensitive) &&
@@ -92,8 +88,8 @@ mbgl::Value CollatorExpression::serialize() const {
     std::unordered_map<std::string, mbgl::Value> options;
     options["case-sensitive"] = caseSensitive->serialize();
     options["diacritic-sensitive"] = diacriticSensitive->serialize();
-    if (locale.get()) {
-        options["locale"] = locale->serialize();
+    if (locale) {
+        options["locale"] = (*locale)->serialize();
     }
     return std::vector<mbgl::Value>{{ std::string("collator"), options }};
 }
@@ -108,8 +104,8 @@ EvaluationResult CollatorExpression::evaluate(const EvaluationContext& params) c
         return diacriticSensitiveResult.error();
     }
     
-    if (locale.get()) {
-        auto localeResult = locale->evaluate(params);
+    if (locale) {
+        auto localeResult = (*locale)->evaluate(params);
         if (!localeResult) {
             return localeResult.error();
         }
diff --git a/src/mbgl/style/expression/equals.cpp b/src/mbgl/style/expression/equals.cpp
index 9a31338807..665dfc1f21 100644
--- a/src/mbgl/style/expression/equals.cpp
+++ b/src/mbgl/style/expression/equals.cpp
@@ -5,7 +5,7 @@ namespace mbgl {
 namespace style {
 namespace expression {
 
-Equals::Equals(std::unique_ptr<Expression> lhs_, std::unique_ptr<Expression> rhs_, std::unique_ptr<Expression> collator_, bool negate_)
+Equals::Equals(std::unique_ptr<Expression> lhs_, std::unique_ptr<Expression> rhs_, optional<std::unique_ptr<Expression>> collator_, bool negate_)
     : Expression(type::Boolean),
       lhs(std::move(lhs_)),
       rhs(std::move(rhs_)),
@@ -22,8 +22,8 @@ EvaluationResult Equals::evaluate(const EvaluationContext& params) const {
 
     bool result;
     
-    if (collator.get()) {
-        auto collatorResult = collator->evaluate(params);
+    if (collator) {
+        auto collatorResult = (*collator)->evaluate(params);
         const Collator& c = collatorResult->get<Collator>();
         result = c.compare(lhsResult->get<std::string>(), rhsResult->get<std::string>()) == 0;
     } else {
@@ -38,8 +38,8 @@ EvaluationResult Equals::evaluate(const EvaluationContext& params) const {
 void Equals::eachChild(const std::function<void(const Expression&)>& visit) const {
     visit(*lhs);
     visit(*rhs);
-    if (collator.get()) {
-        visit(*collator);
+    if (collator) {
+        visit(**collator);
     }
 }
 
@@ -92,18 +92,17 @@ ParseResult Equals::parse(const Convertible& value, ParsingContext& ctx) {
         return ParseResult();
     }
     
-    std::unique_ptr<Expression> collatorResult;
+    ParseResult collatorParseResult;
     if (length == 4) {
         if (lhsType != type::String && rhsType != type::String) {
             ctx.error("Cannot use collator to compare non-string types.");
             return ParseResult();
         }
-        ParseResult collatorParseResult = ctx.parse(arrayMember(value, 3), 3, {type::Collator});
+        collatorParseResult = ctx.parse(arrayMember(value, 3), 3, {type::Collator});
         if (!collatorParseResult) return ParseResult();
-        collatorResult = std::move(*collatorParseResult);
     }
 
-    return ParseResult(std::make_unique<Equals>(std::move(*lhs), std::move(*rhs), std::move(collatorResult), negate));
+    return ParseResult(std::make_unique<Equals>(std::move(*lhs), std::move(*rhs), std::move(collatorParseResult), negate));
 }
 
 } // namespace expression
diff --git a/src/mbgl/style/expression/value.cpp b/src/mbgl/style/expression/value.cpp
index 746e1f0179..2d17667b3c 100644
--- a/src/mbgl/style/expression/value.cpp
+++ b/src/mbgl/style/expression/value.cpp
@@ -45,9 +45,9 @@ void writeJSON(rapidjson::Writer<rapidjson::StringBuffer>& writer, const Value&
         [&] (const std::string& s) { writer.String(s); },
         [&] (const Color& c) { writer.String(c.stringify()); },
         [&] (const Collator&) {
-            // TODO: Collators are excluded from constant folding and there's no Literal parser
+            // Collators are excluded from constant folding and there's no Literal parser
             // for them so there shouldn't be any way to serialize this value.
-            // Is there a better way to omit this?
+            assert(false);
         },
         [&] (const std::vector<Value>& arr) {
             writer.StartArray();
@@ -121,8 +121,11 @@ mbgl::Value ValueConverter<mbgl::Value>::fromExpressionValue(const Value& value)
                 array[3],
             };
         },
-        [&](const Collator& collator)->mbgl::Value {
-            return collator.serialize();
+        [&](const Collator&)->mbgl::Value {
+            // Collators are excluded from constant folding and there's no Literal parser
+            // for them so there shouldn't be any way to serialize this value.
+            assert(false);
+            return mbgl::Value();
         },
         [&](const std::vector<Value>& values)->mbgl::Value {
             std::vector<mbgl::Value> converted;