diff --git a/benchmark/parse/filter.benchmark.cpp b/benchmark/parse/filter.benchmark.cpp index 3918f0353..d650cb72c 100644 --- a/benchmark/parse/filter.benchmark.cpp +++ b/benchmark/parse/filter.benchmark.cpp @@ -14,7 +14,8 @@ using namespace mbgl; style::Filter parse(const char* expression) { rapidjson::GenericDocument, rapidjson::CrtAllocator> doc; doc.Parse<0>(expression); - return *style::conversion::convert(doc); + style::conversion::Error error; + return *style::conversion::convert(doc, error); } static void Parse_Filter(benchmark::State& state) { diff --git a/cmake/core-files.cmake b/cmake/core-files.cmake index 45a3ed25b..e98f99911 100644 --- a/cmake/core-files.cmake +++ b/cmake/core-files.cmake @@ -276,8 +276,6 @@ set(MBGL_CORE_FILES src/mbgl/style/possibly_evaluated_property_value.hpp src/mbgl/style/property_evaluation_parameters.hpp src/mbgl/style/property_evaluator.hpp - src/mbgl/style/property_parsing.cpp - src/mbgl/style/property_parsing.hpp src/mbgl/style/rapidjson_conversion.hpp src/mbgl/style/source.cpp src/mbgl/style/source_impl.cpp diff --git a/include/mbgl/style/conversion.hpp b/include/mbgl/style/conversion.hpp index e53adcb94..d6fb3a6dd 100644 --- a/include/mbgl/style/conversion.hpp +++ b/include/mbgl/style/conversion.hpp @@ -1,6 +1,6 @@ #pragma once -#include +#include #include @@ -21,11 +21,11 @@ namespace conversion { A single template function serves as the public interface: template - Result convert(const V& value); + optional convert(const V& value, Error& error); - Where `T` is one of the above types. If the conversion fails, the `Error` variant of `Result` is - returned, which includes diagnostic text suitable for presentation to a library user. Otherwise, - the `T` variant of `Result` is returned. + Where `T` is one of the above types. If the conversion fails, the result is empty, and the + error parameter includes diagnostic text suitable for presentation to a library user. Otherwise, + a filled optional is returned. The implementation of `convert` requires that the following are legal expressions for a value `v` of type `const V&`: @@ -57,37 +57,12 @@ namespace conversion { struct Error { std::string message; }; -template -class Result : private variant { -public: - using variant::variant; - - explicit operator bool() const { - return this->template is(); - } - - T& operator*() { - assert(this->template is()); - return this->template get(); - } - - const T& operator*() const { - assert(this->template is()); - return this->template get(); - } - - const Error& error() const { - assert(this->template is()); - return this->template get(); - } -}; - template struct Converter; template -Result convert(const V& value, Args&&...args) { - return Converter()(value, std::forward(args)...); +optional convert(const V& value, Error& error, Args&&...args) { + return Converter()(value, error, std::forward(args)...); } } // namespace conversion diff --git a/include/mbgl/style/conversion/constant.hpp b/include/mbgl/style/conversion/constant.hpp index 05bf968f4..1e1fdc2ee 100644 --- a/include/mbgl/style/conversion/constant.hpp +++ b/include/mbgl/style/conversion/constant.hpp @@ -16,10 +16,11 @@ namespace conversion { template <> struct Converter { template - Result operator()(const V& value) const { + optional operator()(const V& value, Error& error) const { optional converted = toBool(value); if (!converted) { - return Error { "value must be a boolean" }; + error = { "value must be a boolean" }; + return {}; } return *converted; } @@ -28,10 +29,11 @@ struct Converter { template <> struct Converter { template - Result operator()(const V& value) const { + optional operator()(const V& value, Error& error) const { optional converted = toNumber(value); if (!converted) { - return Error { "value must be a number" }; + error = { "value must be a number" }; + return {}; } return *converted; } @@ -40,10 +42,11 @@ struct Converter { template <> struct Converter { template - Result operator()(const V& value) const { + optional operator()(const V& value, Error& error) const { optional converted = toString(value); if (!converted) { - return Error { "value must be a string" }; + error = { "value must be a string" }; + return {}; } return *converted; } @@ -52,15 +55,17 @@ struct Converter { template struct Converter::value>> { template - Result operator()(const V& value) const { + optional operator()(const V& value, Error& error) const { optional string = toString(value); if (!string) { - return Error { "value must be a string" }; + error = { "value must be a string" }; + return {}; } const auto result = Enum::toEnum(*string); if (!result) { - return Error { "value must be a valid enumeration value" }; + error = { "value must be a valid enumeration value" }; + return {}; } return *result; @@ -70,15 +75,17 @@ struct Converter::value>> { template <> struct Converter { template - Result operator()(const V& value) const { + optional operator()(const V& value, Error& error) const { optional string = toString(value); if (!string) { - return Error { "value must be a string" }; + error = { "value must be a string" }; + return {}; } optional color = Color::parse(*string); if (!color) { - return Error { "value must be a valid color" }; + error = { "value must be a valid color" }; + return {}; } return *color; @@ -88,15 +95,17 @@ struct Converter { template <> struct Converter> { template - Result> operator()(const V& value) const { + optional> operator()(const V& value, Error& error) const { if (!isArray(value) || arrayLength(value) != 2) { - return Error { "value must be an array of two numbers" }; + error = { "value must be an array of two numbers" }; + return {}; } optional first = toNumber(arrayMember(value, 0)); optional second = toNumber(arrayMember(value, 1)); if (!first || !second) { - return Error { "value must be an array of two numbers" }; + error = { "value must be an array of two numbers" }; + return {}; } return std::array {{ *first, *second }}; @@ -106,9 +115,10 @@ struct Converter> { template <> struct Converter> { template - Result> operator()(const V& value) const { + optional> operator()(const V& value, Error& error) const { if (!isArray(value) || arrayLength(value) != 4) { - return Error { "value must be an array of four numbers" }; + error = { "value must be an array of four numbers" }; + return {}; } optional first = toNumber(arrayMember(value, 0)); @@ -116,7 +126,8 @@ struct Converter> { optional third = toNumber(arrayMember(value, 2)); optional fourth = toNumber(arrayMember(value, 3)); if (!first || !second) { - return Error { "value must be an array of four numbers" }; + error = { "value must be an array of four numbers" }; + return {}; } return std::array {{ *first, *second, *third, *fourth }}; @@ -126,9 +137,10 @@ struct Converter> { template <> struct Converter> { template - Result> operator()(const V& value) const { + optional> operator()(const V& value, Error& error) const { if (!isArray(value)) { - return Error { "value must be an array" }; + error = { "value must be an array" }; + return {}; } std::vector result; @@ -137,7 +149,8 @@ struct Converter> { for (std::size_t i = 0; i < arrayLength(value); ++i) { optional number = toNumber(arrayMember(value, i)); if (!number) { - return Error { "value must be an array of numbers" }; + error = { "value must be an array of numbers" }; + return {}; } result.push_back(*number); } @@ -149,9 +162,10 @@ struct Converter> { template <> struct Converter> { template - Result> operator()(const V& value) const { + optional> operator()(const V& value, Error& error) const { if (!isArray(value)) { - return Error { "value must be an array" }; + error = { "value must be an array" }; + return {}; } std::vector result; @@ -160,7 +174,8 @@ struct Converter> { for (std::size_t i = 0; i < arrayLength(value); ++i) { optional string = toString(arrayMember(value, i)); if (!string) { - return Error { "value must be an array of strings" }; + error = { "value must be an array of strings" }; + return {}; } result.push_back(*string); } diff --git a/include/mbgl/style/conversion/data_driven_property_value.hpp b/include/mbgl/style/conversion/data_driven_property_value.hpp index 83f44fdb2..79b15dcfb 100644 --- a/include/mbgl/style/conversion/data_driven_property_value.hpp +++ b/include/mbgl/style/conversion/data_driven_property_value.hpp @@ -12,29 +12,29 @@ namespace conversion { template struct Converter> { template - Result> operator()(const V& value) const { + optional> operator()(const V& value, Error& error) const { if (isUndefined(value)) { - return {}; + return DataDrivenPropertyValue(); } else if (!isObject(value)) { - Result constant = convert(value); + optional constant = convert(value, error); if (!constant) { - return constant.error(); + return {}; } return DataDrivenPropertyValue(*constant); } else if (!objectMember(value, "property")) { - Result> function = convert>(value); + optional> function = convert>(value, error); if (!function) { - return function.error(); + return {}; } return DataDrivenPropertyValue(*function); } else { - Result> composite = convert>(value); + optional> composite = convert>(value, error); if (composite) { return DataDrivenPropertyValue(*composite); } - Result> source = convert>(value); + optional> source = convert>(value, error); if (!source) { - return source.error(); + return {}; } return DataDrivenPropertyValue(*source); } diff --git a/include/mbgl/style/conversion/filter.hpp b/include/mbgl/style/conversion/filter.hpp index 2c4eeb4ac..1f8f0fd16 100644 --- a/include/mbgl/style/conversion/filter.hpp +++ b/include/mbgl/style/conversion/filter.hpp @@ -12,65 +12,71 @@ template <> struct Converter { public: template - Result operator()(const V& value) const { + optional operator()(const V& value, Error& error) const { if (!isArray(value)) { - return Error { "filter expression must be an array" }; + error = { "filter expression must be an array" }; + return {}; } if (arrayLength(value) < 1) { - return Error { "filter expression must have at least 1 element" }; + error = { "filter expression must have at least 1 element" }; + return {}; } optional op = toString(arrayMember(value, 0)); if (!op) { - return Error { "filter operator must be a string" }; + error = { "filter operator must be a string" }; + return {}; } if (*op == "==") { - return convertEqualityFilter(value); + return convertEqualityFilter(value, error); } else if (*op == "!=") { - return convertEqualityFilter(value); + return convertEqualityFilter(value, error); } else if (*op == ">") { - return convertBinaryFilter(value); + return convertBinaryFilter(value, error); } else if (*op == ">=") { - return convertBinaryFilter(value); + return convertBinaryFilter(value, error); } else if (*op == "<") { - return convertBinaryFilter(value); + return convertBinaryFilter(value, error); } else if (*op == "<=") { - return convertBinaryFilter(value); + return convertBinaryFilter(value, error); } else if (*op == "in") { - return convertSetFilter(value); + return convertSetFilter(value, error); } else if (*op == "!in") { - return convertSetFilter(value); + return convertSetFilter(value, error); } else if (*op == "all") { - return convertCompoundFilter(value); + return convertCompoundFilter(value, error); } else if (*op == "any") { - return convertCompoundFilter(value); + return convertCompoundFilter(value, error); } else if (*op == "none") { - return convertCompoundFilter(value); + return convertCompoundFilter(value, error); } else if (*op == "has") { - return convertUnaryFilter(value); + return convertUnaryFilter(value, error); } else if (*op == "!has") { - return convertUnaryFilter(value); + return convertUnaryFilter(value, error); } - return Error { "filter operator must be one of \"==\", \"!=\", \">\", \">=\", \"<\", \"<=\", \"in\", \"!in\", \"all\", \"any\", \"none\", \"has\", or \"!has\"" }; + error = { "filter operator must be one of \"==\", \"!=\", \">\", \">=\", \"<\", \"<=\", \"in\", \"!in\", \"all\", \"any\", \"none\", \"has\", or \"!has\"" }; + return {}; } private: - Result normalizeValue(const optional& value) const { + optional normalizeValue(const optional& value, Error& error) const { if (!value) { - return Error { "filter expression value must be a boolean, number, or string" }; + error = { "filter expression value must be a boolean, number, or string" }; + return {}; } else { return *value; } } template - Result toFeatureType(const V& value) const { + optional toFeatureType(const V& value, Error& error) const { optional type = toString(value); if (!type) { - return Error { "value for $type filter must be a string" }; + error = { "value for $type filter must be a string" }; + return {}; } else if (*type == "Point") { return FeatureType::Point; } else if (*type == "LineString") { @@ -78,162 +84,173 @@ private: } else if (*type == "Polygon") { return FeatureType::Polygon; } else { - return Error { "value for $type filter must be Point, LineString, or Polygon" }; + error = { "value for $type filter must be Point, LineString, or Polygon" }; + return {}; } } template - Result toFeatureIdentifier(const V& value) const { + optional toFeatureIdentifier(const V& value, Error& error) const { optional identifier = toValue(value); if (!identifier) { - return Error { "filter expression value must be a boolean, number, or string" }; + error = { "filter expression value must be a boolean, number, or string" }; + return {}; } else { return (*identifier).match( - [] (uint64_t t) -> Result { return t; }, - [] ( int64_t t) -> Result { return t; }, - [] ( double t) -> Result { return t; }, - [] (const std::string& t) -> Result { return t; }, - [] (const auto&) -> Result { - return Error { "filter expression value must be a boolean, number, or string" }; + [] (uint64_t t) -> optional { return { t }; }, + [] ( int64_t t) -> optional { return { t }; }, + [] ( double t) -> optional { return { t }; }, + [] (const std::string& t) -> optional { return { t }; }, + [&] (const auto&) -> optional { + error = { "filter expression value must be a boolean, number, or string" }; + return {}; }); } } template - Result convertUnaryFilter(const V& value) const { + optional convertUnaryFilter(const V& value, Error& error) const { if (arrayLength(value) < 2) { - return Error { "filter expression must have 2 elements" }; + error = { "filter expression must have 2 elements" }; + return {}; } optional key = toString(arrayMember(value, 1)); if (!key) { - return Error { "filter expression key must be a string" }; + error = { "filter expression key must be a string" }; + return {}; } if (*key == "$id") { - return IdentifierFilterType {}; + return { IdentifierFilterType {} }; } else { - return FilterType { *key }; + return { FilterType { *key } }; } } template - Result convertEqualityFilter(const V& value) const { + optional convertEqualityFilter(const V& value, Error& error) const { if (arrayLength(value) < 3) { - return Error { "filter expression must have 3 elements" }; + error = { "filter expression must have 3 elements" }; + return {}; } optional key = toString(arrayMember(value, 1)); if (!key) { - return Error { "filter expression key must be a string" }; + error = { "filter expression key must be a string" }; + return {}; } if (*key == "$type") { - Result filterValue = toFeatureType(arrayMember(value, 2)); + optional filterValue = toFeatureType(arrayMember(value, 2), error); if (!filterValue) { - return filterValue.error(); + return {}; } - return TypeFilterType { *filterValue }; + return { TypeFilterType { *filterValue } }; } else if (*key == "$id") { - Result filterValue = toFeatureIdentifier(arrayMember(value, 2)); + optional filterValue = toFeatureIdentifier(arrayMember(value, 2), error); if (!filterValue) { - return filterValue.error(); + return {}; } - return IdentifierFilterType { *filterValue }; + return { IdentifierFilterType { *filterValue } }; } else { - Result filterValue = normalizeValue(toValue(arrayMember(value, 2))); + optional filterValue = normalizeValue(toValue(arrayMember(value, 2)), error); if (!filterValue) { - return filterValue.error(); + return {}; } - return FilterType { *key, *filterValue }; + return { FilterType { *key, *filterValue } }; } } template - Result convertBinaryFilter(const V& value) const { + optional convertBinaryFilter(const V& value, Error& error) const { if (arrayLength(value) < 3) { - return Error { "filter expression must have 3 elements" }; + error = { "filter expression must have 3 elements" }; + return {}; } optional key = toString(arrayMember(value, 1)); if (!key) { - return Error { "filter expression key must be a string" }; + error = { "filter expression key must be a string" }; + return {}; } - Result filterValue = normalizeValue(toValue(arrayMember(value, 2))); + optional filterValue = normalizeValue(toValue(arrayMember(value, 2)), error); if (!filterValue) { - return filterValue.error(); + return {}; } - return FilterType { *key, *filterValue }; + return { FilterType { *key, *filterValue } }; } template - Result convertSetFilter(const V& value) const { + optional convertSetFilter(const V& value, Error& error) const { if (arrayLength(value) < 2) { - return Error { "filter expression must at least 2 elements" }; + error = { "filter expression must at least 2 elements" }; + return {}; } optional key = toString(arrayMember(value, 1)); if (!key) { - return Error { "filter expression key must be a string" }; + error = { "filter expression key must be a string" }; + return {}; } if (*key == "$type") { std::vector values; for (std::size_t i = 2; i < arrayLength(value); ++i) { - Result filterValue = toFeatureType(arrayMember(value, i)); + optional filterValue = toFeatureType(arrayMember(value, i), error); if (!filterValue) { - return filterValue.error(); + return {}; } values.push_back(*filterValue); } - return TypeFilterType { std::move(values) }; + return { TypeFilterType { std::move(values) } }; } else if (*key == "$id") { std::vector values; for (std::size_t i = 2; i < arrayLength(value); ++i) { - Result filterValue = toFeatureIdentifier(arrayMember(value, i)); + optional filterValue = toFeatureIdentifier(arrayMember(value, i), error); if (!filterValue) { - return filterValue.error(); + return {}; } values.push_back(*filterValue); } - return IdentifierFilterType { std::move(values) }; + return { IdentifierFilterType { std::move(values) } }; } else { std::vector values; for (std::size_t i = 2; i < arrayLength(value); ++i) { - Result filterValue = normalizeValue(toValue(arrayMember(value, i))); + optional filterValue = normalizeValue(toValue(arrayMember(value, i)), error); if (!filterValue) { - return filterValue.error(); + return {}; } values.push_back(*filterValue); } - return FilterType { *key, std::move(values) }; + return { FilterType { *key, std::move(values) } }; } } template - Result convertCompoundFilter(const V& value) const { + optional convertCompoundFilter(const V& value, Error& error) const { std::vector filters; for (std::size_t i = 1; i < arrayLength(value); ++i) { - Result element = operator()(arrayMember(value, i)); + optional element = operator()(arrayMember(value, i), error); if (!element) { - return element.error(); + return {}; } filters.push_back(*element); } - return FilterType { std::move(filters) }; + return { FilterType { std::move(filters) } }; } }; diff --git a/include/mbgl/style/conversion/function.hpp b/include/mbgl/style/conversion/function.hpp index a5979e679..fa8af1e2b 100644 --- a/include/mbgl/style/conversion/function.hpp +++ b/include/mbgl/style/conversion/function.hpp @@ -12,18 +12,21 @@ namespace style { namespace conversion { template -Result> convertStops(const V& value) { +optional> convertStops(const V& value, Error& error) { auto stopsValue = objectMember(value, "stops"); if (!stopsValue) { - return Error { "function value must specify stops" }; + error = { "function value must specify stops" }; + return {}; } if (!isArray(*stopsValue)) { - return Error { "function stops must be an array" }; + error = { "function stops must be an array" }; + return {}; } if (arrayLength(*stopsValue) == 0) { - return Error { "function must have at least one stop" }; + error = { "function must have at least one stop" }; + return {}; } std::map stops; @@ -31,21 +34,23 @@ Result> convertStops(const V& value) { const auto& stopValue = arrayMember(*stopsValue, i); if (!isArray(stopValue)) { - return Error { "function stop must be an array" }; + error = { "function stop must be an array" }; + return {}; } if (arrayLength(stopValue) != 2) { - return Error { "function stop must have two elements" }; + error = { "function stop must have two elements" }; + return {}; } - Result d = convert(arrayMember(stopValue, 0)); + optional d = convert(arrayMember(stopValue, 0), error); if (!d) { - return d.error(); + return {}; } - Result r = convert(arrayMember(stopValue, 1)); + optional r = convert(arrayMember(stopValue, 1), error); if (!r) { - return r.error(); + return {}; } stops.emplace(*d, *r); @@ -59,10 +64,10 @@ struct Converter> { static constexpr const char * type = "exponential"; template - Result> operator()(const V& value) const { - auto stops = convertStops(value); + optional> operator()(const V& value, Error& error) const { + auto stops = convertStops(value, error); if (!stops) { - return stops.error(); + return {}; } auto baseValue = objectMember(value, "base"); @@ -72,7 +77,8 @@ struct Converter> { optional base = toNumber(*baseValue); if (!base) { - return Error { "function base must be a number"}; + error = { "function base must be a number"}; + return {}; } return ExponentialStops(*stops, *base); @@ -84,10 +90,10 @@ struct Converter> { static constexpr const char * type = "interval"; template - Result> operator()(const V& value) const { - auto stops = convertStops(value); + optional> operator()(const V& value, Error& error) const { + auto stops = convertStops(value, error); if (!stops) { - return stops.error(); + return {}; } return IntervalStops(*stops); } @@ -96,23 +102,24 @@ struct Converter> { template <> struct Converter { template - Result operator()(const V& value) const { + optional operator()(const V& value, Error& error) const { auto b = toBool(value); if (b) { - return *b; + return { *b }; } auto n = toNumber(value); if (n) { - return int64_t(*n); + return { int64_t(*n) }; } auto s = toString(value); if (s) { - return *s; + return { *s }; } - return Error { "stop domain value must be a number, string, or boolean" }; + error = { "stop domain value must be a number, string, or boolean" }; + return {}; } }; @@ -121,10 +128,10 @@ struct Converter> { static constexpr const char * type = "categorical"; template - Result> operator()(const V& value) const { - auto stops = convertStops(value); + optional> operator()(const V& value, Error& error) const { + auto stops = convertStops(value, error); if (!stops) { - return stops.error(); + return {}; } return CategoricalStops( std::map((*stops).begin(), (*stops).end())); @@ -136,7 +143,7 @@ struct Converter> { static constexpr const char * type = "identity"; template - Result> operator()(const V&) const { + optional> operator()(const V&, Error&) const { return IdentityStops(); } }; @@ -148,7 +155,7 @@ template struct StopsConverter> { public: template - Result> operator()(const V& value) const { + optional> operator()(const V& value, Error& error) const { std::string type = util::Interpolatable ? "exponential" : "interval"; auto typeValue = objectMember(value, "type"); @@ -156,16 +163,18 @@ public: type = *toString(*typeValue); } - optional>> result; + bool matched = false; + optional> result; // Workaround for https://gcc.gnu.org/bugzilla/show_bug.cgi?id=47226 auto tryConvert = [&] (auto* tp) { using Stops = std::decay_t; if (type == Converter::type) { - auto stops = convert(value); - result = stops - ? Result>(*stops) - : Result>(stops.error()); + matched = true; + optional stops = convert(value, error); + if (stops) { + result = variant(*stops); + } } }; @@ -173,25 +182,27 @@ public: (tryConvert((Ts*)nullptr), 0)... }); - if (!result) { - return Error { "unsupported function type" }; + if (!matched) { + error = { "unsupported function type" }; + return {}; } - return *result; + return result; } }; template struct Converter> { template - Result> operator()(const V& value) const { + optional> operator()(const V& value, Error& error) const { if (!isObject(value)) { - return Error { "function must be an object" }; + error = { "function must be an object" }; + return {}; } - auto stops = StopsConverter::Stops>()(value); + auto stops = StopsConverter::Stops>()(value, error); if (!stops) { - return stops.error(); + return {}; } return CameraFunction(*stops); @@ -199,46 +210,50 @@ struct Converter> { }; template -Result> convertDefaultValue(const V& value) { +optional> convertDefaultValue(const V& value, Error& error) { auto defaultValueValue = objectMember(value, "default"); if (!defaultValueValue) { + return optional(); + } + + auto defaultValue = convert(*defaultValueValue, error); + if (!defaultValue) { + error = { "wrong type for \"default\": " + error.message }; return {}; } - auto defaultValue = convert(*defaultValueValue); - if (!defaultValue) { - return Error { "wrong type for \"default\": " + defaultValue.error().message }; - } - - return *defaultValue; + return { *defaultValue }; } template struct Converter> { template - Result> operator()(const V& value) const { + optional> operator()(const V& value, Error& error) const { if (!isObject(value)) { - return Error { "function must be an object" }; + error = { "function must be an object" }; + return {}; } auto propertyValue = objectMember(value, "property"); if (!propertyValue) { - return Error { "function must specify property" }; + error = { "function must specify property" }; + return {}; } auto propertyString = toString(*propertyValue); if (!propertyString) { - return Error { "function property must be a string" }; + error = { "function property must be a string" }; + return {}; } - auto stops = StopsConverter::Stops>()(value); + auto stops = StopsConverter::Stops>()(value, error); if (!stops) { - return stops.error(); + return {}; } - auto defaultValue = convertDefaultValue(value); + auto defaultValue = convertDefaultValue(value, error); if (!defaultValue) { - return defaultValue.error(); + return {}; } return SourceFunction(*propertyString, *stops, *defaultValue); @@ -253,29 +268,32 @@ struct CompositeValue : std::pair { template struct Converter> { template - Result> operator()(const V& value) const { + optional> operator()(const V& value, Error& error) const { if (!isObject(value)) { - return Error { "stop must be an object" }; + error = { "stop must be an object" }; + return {}; } auto zoomValue = objectMember(value, "zoom"); if (!zoomValue) { - return Error { "stop must specify zoom" }; + error = { "stop must specify zoom" }; + return {}; } auto propertyValue = objectMember(value, "value"); if (!propertyValue) { - return Error { "stop must specify value" }; + error = { "stop must specify value" }; + return {}; } - Result z = convert(*zoomValue); + optional z = convert(*zoomValue, error); if (!z) { - return z.error(); + return {}; } - Result s = convert(*propertyValue); + optional s = convert(*propertyValue, error); if (!s) { - return s.error(); + return {}; } return CompositeValue { *z, *s }; @@ -287,10 +305,10 @@ struct Converter> { static constexpr const char * type = "exponential"; template - Result> operator()(const V& value) const { - auto stops = convertStops, T>(value); + optional> operator()(const V& value, Error& error) const { + auto stops = convertStops, T>(value, error); if (!stops) { - return stops.error(); + return {}; } auto base = 1.0f; @@ -313,10 +331,10 @@ struct Converter> { static constexpr const char * type = "interval"; template - Result> operator()(const V& value) const { - auto stops = convertStops, T>(value); + optional> operator()(const V& value, Error& error) const { + auto stops = convertStops, T>(value, error); if (!stops) { - return stops.error(); + return {}; } std::map> convertedStops; @@ -333,10 +351,10 @@ struct Converter> { static constexpr const char * type = "categorical"; template - Result> operator()(const V& value) const { - auto stops = convertStops, T>(value); + optional> operator()(const V& value, Error& error) const { + auto stops = convertStops, T>(value, error); if (!stops) { - return stops.error(); + return {}; } std::map> convertedStops; @@ -351,29 +369,32 @@ struct Converter> { template struct Converter> { template - Result> operator()(const V& value) const { + optional> operator()(const V& value, Error& error) const { if (!isObject(value)) { - return Error { "function must be an object" }; + error = { "function must be an object" }; + return {}; } auto propertyValue = objectMember(value, "property"); if (!propertyValue) { - return Error { "function must specify property" }; + error = { "function must specify property" }; + return {}; } auto propertyString = toString(*propertyValue); if (!propertyString) { - return Error { "function property must be a string" }; + error = { "function property must be a string" }; + return {}; } - auto stops = StopsConverter::Stops>()(value); + auto stops = StopsConverter::Stops>()(value, error); if (!stops) { - return stops.error(); + return {}; } - auto defaultValue = convertDefaultValue(value); + auto defaultValue = convertDefaultValue(value, error); if (!defaultValue) { - return defaultValue.error(); + return {}; } return CompositeFunction(*propertyString, *stops, *defaultValue); diff --git a/include/mbgl/style/conversion/geojson.hpp b/include/mbgl/style/conversion/geojson.hpp index ba10b3ecc..6e6e97dbc 100644 --- a/include/mbgl/style/conversion/geojson.hpp +++ b/include/mbgl/style/conversion/geojson.hpp @@ -8,7 +8,7 @@ namespace style { namespace conversion { template -Result convertGeoJSON(const V& value); +optional convertGeoJSON(const V& value, Error& error); } // namespace conversion } // namespace style diff --git a/include/mbgl/style/conversion/geojson_options.hpp b/include/mbgl/style/conversion/geojson_options.hpp index 880090b40..19383d90c 100644 --- a/include/mbgl/style/conversion/geojson_options.hpp +++ b/include/mbgl/style/conversion/geojson_options.hpp @@ -11,7 +11,7 @@ template <> struct Converter { template - Result operator()(const V& value) const { + optional operator()(const V& value, Error& error) const { GeoJSONOptions options; const auto maxzoomValue = objectMember(value, "maxzoom"); @@ -19,7 +19,8 @@ struct Converter { if (toNumber(*maxzoomValue)) { options.maxzoom = static_cast(*toNumber(*maxzoomValue)); } else { - return Error{ "GeoJSON source maxzoom value must be a number" }; + error = { "GeoJSON source maxzoom value must be a number" }; + return {}; } } @@ -28,7 +29,8 @@ struct Converter { if (toNumber(*bufferValue)) { options.buffer = static_cast(*toNumber(*bufferValue)); } else { - return Error{ "GeoJSON source buffer value must be a number" }; + error = { "GeoJSON source buffer value must be a number" }; + return {}; } } @@ -37,7 +39,8 @@ struct Converter { if (toNumber(*toleranceValue)) { options.tolerance = static_cast(*toNumber(*toleranceValue)); } else { - return Error{ "GeoJSON source tolerance value must be a number" }; + error = { "GeoJSON source tolerance value must be a number" }; + return {}; } } @@ -46,7 +49,8 @@ struct Converter { if (toBool(*clusterValue)) { options.cluster = *toBool(*clusterValue); } else { - return Error{ "GeoJSON source cluster value must be a boolean" }; + error = { "GeoJSON source cluster value must be a boolean" }; + return {}; } } @@ -55,7 +59,8 @@ struct Converter { if (toNumber(*clusterMaxZoomValue)) { options.clusterMaxZoom = static_cast(*toNumber(*clusterMaxZoomValue)); } else { - return Error{ "GeoJSON source clusterMaxZoom value must be a number" }; + error = { "GeoJSON source clusterMaxZoom value must be a number" }; + return {}; } } @@ -64,7 +69,8 @@ struct Converter { if (toNumber(*clusterRadiusValue)) { options.clusterRadius = static_cast(*toNumber(*clusterRadiusValue)); } else { - return Error{ "GeoJSON source clusterRadius value must be a number" }; + error = { "GeoJSON source clusterRadius value must be a number" }; + return {}; } } diff --git a/include/mbgl/style/conversion/layer.hpp b/include/mbgl/style/conversion/layer.hpp index 0539dcf9a..efb1df8fe 100644 --- a/include/mbgl/style/conversion/layer.hpp +++ b/include/mbgl/style/conversion/layer.hpp @@ -58,47 +58,53 @@ template <> struct Converter> { public: template - Result> operator()(const V& value) const { + optional> operator()(const V& value, Error& error) const { if (!isObject(value)) { - return Error { "layer must be an object" }; + error = { "layer must be an object" }; + return {}; } auto idValue = objectMember(value, "id"); if (!idValue) { - return Error { "layer must have an id" }; + error = { "layer must have an id" }; + return {}; } optional id = toString(*idValue); if (!id) { - return Error { "layer id must be a string" }; + error = { "layer id must be a string" }; + return {}; } auto typeValue = objectMember(value, "type"); if (!typeValue) { - return Error { "layer must have a type" }; + error = { "layer must have a type" }; + return {}; } optional type = toString(*typeValue); if (!type) { - return Error { "layer type must be a string" }; + error = { "layer type must be a string" }; + return {}; } - Result> converted; + optional> converted; if (*type == "fill") { - converted = convertVectorLayer(*id, value); + converted = convertVectorLayer(*id, value, error); } else if (*type == "line") { - converted = convertVectorLayer(*id, value); + converted = convertVectorLayer(*id, value, error); } else if (*type == "circle") { - converted = convertVectorLayer(*id, value); + converted = convertVectorLayer(*id, value, error); } else if (*type == "symbol") { - converted = convertVectorLayer(*id, value); + converted = convertVectorLayer(*id, value, error); } else if (*type == "raster") { - converted = convertRasterLayer(*id, value); + converted = convertRasterLayer(*id, value, error); } else if (*type == "background") { - converted = convertBackgroundLayer(*id, value); + converted = convertBackgroundLayer(*id, value, error); } else { - return Error { "invalid layer type" }; + error = { "invalid layer type" }; + return {}; } if (!converted) { @@ -111,7 +117,8 @@ public: if (minzoomValue) { optional minzoom = toNumber(*minzoomValue); if (!minzoom) { - return Error { "minzoom must be numeric" }; + error = { "minzoom must be numeric" }; + return {}; } layer->setMinZoom(*minzoom); } @@ -120,7 +127,8 @@ public: if (maxzoomValue) { optional maxzoom = toNumber(*maxzoomValue); if (!maxzoom) { - return Error { "maxzoom must be numeric" }; + error = { "maxzoom must be numeric" }; + return {}; } layer->setMaxZoom(*maxzoom); } @@ -128,19 +136,22 @@ public: auto layoutValue = objectMember(value, "layout"); if (layoutValue) { if (!isObject(*layoutValue)) { - return Error { "layout must be an object" }; + error = { "layout must be an object" }; + return {}; } - optional error = eachMember(*layoutValue, [&] (const std::string& k, const V& v) { + optional error_ = eachMember(*layoutValue, [&] (const std::string& k, const V& v) { return setLayoutProperty(*layer, k, v); }); - if (error) { - return *error; + if (error_) { + error = *error_; + return {}; } } - optional error = setPaintProperties(*layer, value); - if (error) { - return *error; + optional error_ = setPaintProperties(*layer, value); + if (error_) { + error = *error_; + return {}; } return std::move(layer); @@ -148,15 +159,17 @@ public: private: template - Result> convertVectorLayer(const std::string& id, const V& value) const { + optional> convertVectorLayer(const std::string& id, const V& value, Error& error) const { auto sourceValue = objectMember(value, "source"); if (!sourceValue) { - return Error { "layer must have a source" }; + error = { "layer must have a source" }; + return {}; } optional source = toString(*sourceValue); if (!source) { - return Error { "layer source must be a string" }; + error = { "layer source must be a string" }; + return {}; } std::unique_ptr layer = std::make_unique(id, *source); @@ -165,41 +178,44 @@ private: if (sourceLayerValue) { optional sourceLayer = toString(*sourceLayerValue); if (!sourceLayer) { - return Error { "layer source-layer must be a string" }; + error = { "layer source-layer must be a string" }; + return {}; } layer->setSourceLayer(*sourceLayer); } auto filterValue = objectMember(value, "filter"); if (filterValue) { - Result filter = convert(*filterValue); + optional filter = convert(*filterValue, error); if (!filter) { - return filter.error(); + return {}; } layer->setFilter(*filter); } - return std::move(layer); + return { std::move(layer) }; } template - Result> convertRasterLayer(const std::string& id, const V& value) const { + optional> convertRasterLayer(const std::string& id, const V& value, Error& error) const { auto sourceValue = objectMember(value, "source"); if (!sourceValue) { - return Error { "layer must have a source" }; + error = { "layer must have a source" }; + return {}; } optional source = toString(*sourceValue); if (!source) { - return Error { "layer source must be a string" }; + error = { "layer source must be a string" }; + return {}; } - return std::make_unique(id, *source); + return { std::make_unique(id, *source) }; } template - Result> convertBackgroundLayer(const std::string& id, const V&) const { - return std::make_unique(id); + optional> convertBackgroundLayer(const std::string& id, const V&, Error&) const { + return { std::make_unique(id) }; } }; diff --git a/include/mbgl/style/conversion/property_setter.hpp b/include/mbgl/style/conversion/property_setter.hpp index 51ec4778d..1f537f3c4 100644 --- a/include/mbgl/style/conversion/property_setter.hpp +++ b/include/mbgl/style/conversion/property_setter.hpp @@ -26,9 +26,10 @@ optional setLayoutProperty(Layer& layer, const V& value) { return Error { "layer doesn't support this property" }; } - Result typedValue = convert(value); + Error error; + optional typedValue = convert(value, error); if (!typedValue) { - return typedValue.error(); + return error; } (typedLayer->*setter)(*typedValue); @@ -42,9 +43,10 @@ optional setPaintProperty(Layer& layer, const V& value, const optional typedValue = convert(value); + Error error; + optional typedValue = convert(value, error); if (!typedValue) { - return typedValue.error(); + return error; } (typedLayer->*setter)(*typedValue, klass); @@ -58,9 +60,10 @@ optional setTransition(Layer& layer, const V& value, const optional transition = convert(value); + Error error; + optional transition = convert(value, error); if (!transition) { - return transition.error(); + return error; } (typedLayer->*setter)(*transition, klass); @@ -74,9 +77,10 @@ optional setVisibility(Layer& layer, const V& value) { return {}; } - Result visibility = convert(value); + Error error; + optional visibility = convert(value, error); if (!visibility) { - return visibility.error(); + return error; } layer.setVisibility(*visibility); diff --git a/include/mbgl/style/conversion/property_value.hpp b/include/mbgl/style/conversion/property_value.hpp index d5e2a5c3c..f8937da07 100644 --- a/include/mbgl/style/conversion/property_value.hpp +++ b/include/mbgl/style/conversion/property_value.hpp @@ -12,21 +12,21 @@ namespace conversion { template struct Converter> { template - Result> operator()(const V& value) const { + optional> operator()(const V& value, Error& error) const { if (isUndefined(value)) { - return {}; + return PropertyValue(); } else if (isObject(value)) { - Result> function = convert>(value); + optional> function = convert>(value, error); if (!function) { - return function.error(); + return {}; } - return *function; + return { *function }; } else { - Result constant = convert(value); + optional constant = convert(value, error); if (!constant) { - return constant.error(); + return {}; } - return *constant; + return { *constant }; } } }; diff --git a/include/mbgl/style/conversion/source.hpp b/include/mbgl/style/conversion/source.hpp index 6e1b4347c..2371183b1 100644 --- a/include/mbgl/style/conversion/source.hpp +++ b/include/mbgl/style/conversion/source.hpp @@ -17,59 +17,65 @@ template <> struct Converter> { public: template - Result> operator()(const V& value, const std::string& id) const { + optional> operator()(const V& value, Error& error, const std::string& id) const { if (!isObject(value)) { - return Error{ "source must be an object" }; + error = { "source must be an object" }; + return {}; } auto typeValue = objectMember(value, "type"); if (!typeValue) { - return Error{ "source must have a type" }; + error = { "source must have a type" }; + return {}; } optional type = toString(*typeValue); if (!type) { - return Error{ "source type must be a string" }; + error = { "source type must be a string" }; + return {}; } if (*type == "raster") { - return convertRasterSource(id, value); + return convertRasterSource(id, value, error); } else if (*type == "vector") { - return convertVectorSource(id, value); + return convertVectorSource(id, value, error); } else if (*type == "geojson") { - return convertGeoJSONSource(id, value); + return convertGeoJSONSource(id, value, error); } else { - return Error{ "invalid source type" }; + error = { "invalid source type" }; + return {}; } } private: // A tile source can either specify a URL to TileJSON, or inline TileJSON. template - Result> convertURLOrTileset(const V& value) const { + optional> convertURLOrTileset(const V& value, Error& error) const { auto urlVal = objectMember(value, "url"); if (!urlVal) { - Result tileset = convert(value); + optional tileset = convert(value, error); if (!tileset) { - return tileset.error(); + return {}; } - return *tileset; + return { *tileset }; } optional url = toString(*urlVal); if (!url) { - return Error{ "source url must be a string" }; + error = { "source url must be a string" }; + return {}; } - return *url; + return { *url }; } template - Result> convertRasterSource(const std::string& id, - const V& value) const { - Result> urlOrTileset = convertURLOrTileset(value); + optional> convertRasterSource(const std::string& id, + const V& value, + Error& error) const { + optional> urlOrTileset = convertURLOrTileset(value, error); if (!urlOrTileset) { - return urlOrTileset.error(); + return {}; } uint16_t tileSize = util::tileSize; @@ -77,53 +83,58 @@ private: if (tileSizeValue) { optional size = toNumber(*tileSizeValue); if (!size || *size < 0 || *size > std::numeric_limits::max()) { - return Error{ "invalid tileSize" }; + error = { "invalid tileSize" }; + return {}; } tileSize = *size; } - return std::make_unique(id, std::move(*urlOrTileset), tileSize); + return { std::make_unique(id, std::move(*urlOrTileset), tileSize) }; } template - Result> convertVectorSource(const std::string& id, - const V& value) const { - Result> urlOrTileset = convertURLOrTileset(value); + optional> convertVectorSource(const std::string& id, + const V& value, + Error& error) const { + optional> urlOrTileset = convertURLOrTileset(value, error); if (!urlOrTileset) { - return urlOrTileset.error(); + return {}; } - return std::make_unique(id, std::move(*urlOrTileset)); + return { std::make_unique(id, std::move(*urlOrTileset)) }; } template - Result> convertGeoJSONSource(const std::string& id, - const V& value) const { + optional> convertGeoJSONSource(const std::string& id, + const V& value, + Error& error) const { auto dataValue = objectMember(value, "data"); if (!dataValue) { - return Error{ "GeoJSON source must have a data value" }; + error = { "GeoJSON source must have a data value" }; + return {}; } - Result options = convert(value); + optional options = convert(value, error); if (!options) { - return options.error(); + return {}; } auto result = std::make_unique(id, *options); if (isObject(*dataValue)) { - Result geoJSON = convertGeoJSON(*dataValue); + optional geoJSON = convertGeoJSON(*dataValue, error); if (!geoJSON) { - return geoJSON.error(); + return {}; } result->setGeoJSON(std::move(*geoJSON)); } else if (toString(*dataValue)) { result->setURL(*toString(*dataValue)); } else { - return Error{ "GeoJSON data must be a URL or an object" }; + error = { "GeoJSON data must be a URL or an object" }; + return {}; } - return std::move(result); + return { std::move(result) }; } }; diff --git a/include/mbgl/style/conversion/tileset.hpp b/include/mbgl/style/conversion/tileset.hpp index 1955cc16c..377170aa6 100644 --- a/include/mbgl/style/conversion/tileset.hpp +++ b/include/mbgl/style/conversion/tileset.hpp @@ -11,22 +11,25 @@ template <> struct Converter { public: template - Result operator()(const V& value) const { + optional operator()(const V& value, Error& error) const { Tileset result; auto tiles = objectMember(value, "tiles"); if (!tiles) { - return Error { "source must have tiles" }; + error = { "source must have tiles" }; + return {}; } if (!isArray(*tiles)) { - return Error { "source tiles must be an array" }; + error = { "source tiles must be an array" }; + return {}; } for (std::size_t i = 0; i < arrayLength(*tiles); i++) { optional urlTemplate = toString(arrayMember(*tiles, i)); if (!urlTemplate) { - return Error { "source tiles member must be a string" }; + error = { "source tiles member must be a string" }; + return {}; } result.tiles.push_back(std::move(*urlTemplate)); } @@ -43,7 +46,8 @@ public: if (minzoomValue) { optional minzoom = toNumber(*minzoomValue); if (!minzoom || *minzoom < 0 || *minzoom > std::numeric_limits::max()) { - return Error { "invalid minzoom" }; + error = { "invalid minzoom" }; + return {}; } result.zoomRange.min = *minzoom; } @@ -52,7 +56,8 @@ public: if (maxzoomValue) { optional maxzoom = toNumber(*maxzoomValue); if (!maxzoom || *maxzoom < 0 || *maxzoom > std::numeric_limits::max()) { - return Error { "invalid maxzoom" }; + error = { "invalid maxzoom" }; + return {}; } result.zoomRange.max = *maxzoom; } @@ -61,7 +66,8 @@ public: if (attributionValue) { optional attribution = toString(*attributionValue); if (!attribution) { - return Error { "source attribution must be a string" }; + error = { "source attribution must be a string" }; + return {}; } result.attribution = std::move(*attribution); } diff --git a/include/mbgl/style/conversion/transition_options.hpp b/include/mbgl/style/conversion/transition_options.hpp index cdd65cfe9..de8834d57 100644 --- a/include/mbgl/style/conversion/transition_options.hpp +++ b/include/mbgl/style/conversion/transition_options.hpp @@ -11,9 +11,10 @@ template <> struct Converter { public: template - Result operator()(const V& value) const { + optional operator()(const V& value, Error& error) const { if (!isObject(value)) { - return Error { "transition must be an object" }; + error = { "transition must be an object" }; + return {}; } TransitionOptions result; @@ -22,7 +23,8 @@ public: if (duration) { auto number = toNumber(*duration); if (!number) { - return Error { "duration must be a number" }; + error = { "duration must be a number" }; + return {}; } result.duration = { std::chrono::milliseconds(int64_t(*number)) }; } @@ -31,7 +33,8 @@ public: if (delay) { auto number = toNumber(*delay); if (!number) { - return Error { "delay must be a number" }; + error = { "delay must be a number" }; + return {}; } result.delay = { std::chrono::milliseconds(int64_t(*number)) }; } diff --git a/platform/android/src/style/conversion/filter.hpp b/platform/android/src/style/conversion/filter.hpp index fc36d3a04..1f0abcf3a 100644 --- a/platform/android/src/style/conversion/filter.hpp +++ b/platform/android/src/style/conversion/filter.hpp @@ -17,9 +17,10 @@ inline optional toFilter(jni::JNIEnv& env, jni::Array filter; if (jfilter) { Value filterValue(env, jfilter); - auto converted = mbgl::style::conversion::convert(filterValue); + mbgl::style::conversion::Error error; + auto converted = mbgl::style::conversion::convert(filterValue, error); if (!converted) { - mbgl::Log::Error(mbgl::Event::JNI, "Error converting filter: " + converted.error().message); + mbgl::Log::Error(mbgl::Event::JNI, "Error converting filter: " + error.message); } filter = std::move(*converted); } diff --git a/platform/android/src/style/conversion/geojson.hpp b/platform/android/src/style/conversion/geojson.hpp index 415d96f46..6ac6abcd7 100644 --- a/platform/android/src/style/conversion/geojson.hpp +++ b/platform/android/src/style/conversion/geojson.hpp @@ -17,12 +17,13 @@ namespace style { namespace conversion { template <> -Result convertGeoJSON(const mbgl::android::Value& value) { +optional convertGeoJSON(const mbgl::android::Value& value, Error& error) { // Value should be a string wrapped in an object mbgl::android::Value jsonValue = value.get("data"); if(value.isNull()) { - return Error { "no json data found" }; + error = { "no json data found" }; + return {}; } std::string jsonString = value.get("data").toString(); @@ -32,12 +33,14 @@ Result convertGeoJSON(const mbgl::android::Value& value) { if (d.HasParseError()) { std::stringstream message; message << d.GetErrorOffset() << " - " << rapidjson::GetParseError_En(d.GetParseError()); - return Error { message.str() }; + error = { message.str() }; + return {}; } - conversion::Result geoJSON = conversion::convertGeoJSON(d); + optional geoJSON = conversion::convertGeoJSON(d, error); if (!geoJSON) { - return Error { geoJSON.error().message }; + error = { error.message }; + return {}; } return geoJSON; @@ -46,8 +49,8 @@ Result convertGeoJSON(const mbgl::android::Value& value) { template <> struct Converter { - Result operator()(const mbgl::android::Value& value) const { - return convertGeoJSON(value); + optional operator()(const mbgl::android::Value& value, Error& error) const { + return convertGeoJSON(value, error); } }; diff --git a/platform/android/src/style/conversion/url_or_tileset.hpp b/platform/android/src/style/conversion/url_or_tileset.hpp index 4e502324d..00ef913d4 100644 --- a/platform/android/src/style/conversion/url_or_tileset.hpp +++ b/platform/android/src/style/conversion/url_or_tileset.hpp @@ -12,27 +12,25 @@ #include namespace mbgl { -namespace style { -namespace conversion { +namespace android { -template <> -struct Converter> { +// This conversion is expected not to fail because it's used only in contexts where +// the value was originally a String or TileSet object on the Java side. If it fails +// to convert, it's a bug in our serialization or Java-side static typing. +inline variant convertURLOrTileset(const Value& value) { + using namespace mbgl::style::conversion; - template - Result> operator()(const V& value) const { - if (isObject(value)) { - Result tileset = convert(value); - if (!tileset) { - return tileset.error(); - } - return *tileset; - } else { - return *toString(value); + if (isObject(value)) { + Error error; + optional tileset = convert(value, error); + if (!tileset) { + throw std::logic_error(error.message); } + return { *tileset }; + } else { + return { *toString(value) }; } - -}; +} } } -} diff --git a/platform/android/src/style/layers/layer.cpp b/platform/android/src/style/layers/layer.cpp index dbf71fd2a..6e93cf4da 100644 --- a/platform/android/src/style/layers/layer.cpp +++ b/platform/android/src/style/layers/layer.cpp @@ -104,9 +104,10 @@ namespace android { Value wrapped(env, jfilter); Filter filter; - Result converted = convert(wrapped); + Error error; + optional converted = convert(wrapped, error); if (!converted) { - mbgl::Log::Error(mbgl::Event::JNI, "Error setting filter: " + converted.error().message); + mbgl::Log::Error(mbgl::Event::JNI, "Error setting filter: " + error.message); return; } filter = std::move(*converted); diff --git a/platform/android/src/style/sources/geojson_source.cpp b/platform/android/src/style/sources/geojson_source.cpp index 7ab98e47c..0c2d25f9f 100644 --- a/platform/android/src/style/sources/geojson_source.cpp +++ b/platform/android/src/style/sources/geojson_source.cpp @@ -9,7 +9,7 @@ #include "../../conversion/conversion.hpp" #include "../../conversion/collection.hpp" #include "../../geometry/conversion/feature.hpp" -#include "../conversion/url_or_tileset.hpp" + #include #include @@ -18,11 +18,26 @@ namespace mbgl { namespace android { + // This conversion is expected not to fail because it's used only in contexts where + // the value was originally a GeoJsonOptions object on the Java side. If it fails + // to convert, it's a bug in our serialization or Java-side static typing. + static style::GeoJSONOptions convertGeoJSONOptions(jni::JNIEnv& env, jni::Object<> options) { + using namespace mbgl::style::conversion; + if (!options) { + return style::GeoJSONOptions(); + } + Error error; + optional result = convert(Value(env, options), error); + if (!result) { + throw std::logic_error(error.message); + } + return *result; + } + GeoJSONSource::GeoJSONSource(jni::JNIEnv& env, jni::String sourceId, jni::Object<> options) : Source(env, std::make_unique( jni::Make(env, sourceId), - options ? *style::conversion::convert(Value(env, options)) : style::GeoJSONOptions() - ) + convertGeoJSONOptions(env, options)) ) { } @@ -36,9 +51,10 @@ namespace android { using namespace mbgl::style::conversion; // Convert the jni object - Result converted = convert(Value(env, json)); + Error error; + optional converted = convert(Value(env, json), error); if(!converted) { - mbgl::Log::Error(mbgl::Event::JNI, "Error setting geo json: " + converted.error().message); + mbgl::Log::Error(mbgl::Event::JNI, "Error setting geo json: " + error.message); return; } diff --git a/platform/android/src/style/sources/raster_source.cpp b/platform/android/src/style/sources/raster_source.cpp index 42ac4cda9..0234901a7 100644 --- a/platform/android/src/style/sources/raster_source.cpp +++ b/platform/android/src/style/sources/raster_source.cpp @@ -16,7 +16,7 @@ namespace android { env, std::make_unique( jni::Make(env, sourceId), - *style::conversion::convert>(Value(env, urlOrTileSet)), + convertURLOrTileset(Value(env, urlOrTileSet)), tileSize ) ) { diff --git a/platform/android/src/style/sources/vector_source.cpp b/platform/android/src/style/sources/vector_source.cpp index 67777f50d..4852a9b84 100644 --- a/platform/android/src/style/sources/vector_source.cpp +++ b/platform/android/src/style/sources/vector_source.cpp @@ -23,7 +23,7 @@ namespace android { env, std::make_unique( jni::Make(env, sourceId), - *style::conversion::convert>(Value(env, urlOrTileSet)) + convertURLOrTileset(Value(env, urlOrTileSet)) ) ) { } diff --git a/platform/darwin/src/MGLStyleValue_Private.h b/platform/darwin/src/MGLStyleValue_Private.h index 90d6622c9..263b54d7e 100644 --- a/platform/darwin/src/MGLStyleValue_Private.h +++ b/platform/darwin/src/MGLStyleValue_Private.h @@ -123,8 +123,9 @@ public: return toMBGLConstantValue((MGLConstantStyleValue *)value); } else if ([value isKindOfClass:[MGLStyleFunction class]]) { auto rawValue = toRawStyleSpecValue((MGLStyleFunction *) value); - auto result = mbgl::style::conversion::convert>(rawValue); - NSCAssert(result, @(result.error().message.c_str())); + mbgl::style::conversion::Error error; + auto result = mbgl::style::conversion::convert>(rawValue, error); + NSCAssert(result, @(error.message.c_str())); return *result; } else { return {}; diff --git a/platform/node/src/node_geojson.hpp b/platform/node/src/node_geojson.hpp index ace4c9142..e4b2ca47e 100644 --- a/platform/node/src/node_geojson.hpp +++ b/platform/node/src/node_geojson.hpp @@ -5,8 +5,9 @@ namespace style { namespace conversion { template <> -Result convertGeoJSON(const v8::Local&) { - return Error { "not implemented" }; +optional convertGeoJSON(const v8::Local&, Error& error) { + error = { "not implemented" }; + return {}; } } // namespace conversion diff --git a/platform/node/src/node_map.cpp b/platform/node/src/node_map.cpp index a3a11a190..881c1b2fb 100644 --- a/platform/node/src/node_map.cpp +++ b/platform/node/src/node_map.cpp @@ -538,9 +538,10 @@ void NodeMap::AddSource(const Nan::FunctionCallbackInfo& info) { return Nan::ThrowTypeError("First argument must be a string"); } - Result> source = convert>(info[1], *Nan::Utf8String(info[0])); + Error error; + mbgl::optional> source = convert>(info[1], error, *Nan::Utf8String(info[0])); if (!source) { - Nan::ThrowTypeError(source.error().message.c_str()); + Nan::ThrowTypeError(error.message.c_str()); return; } @@ -558,9 +559,10 @@ void NodeMap::AddLayer(const Nan::FunctionCallbackInfo& info) { return Nan::ThrowTypeError("One argument required"); } - Result> layer = convert>(info[0]); + Error error; + mbgl::optional> layer = convert>(info[0], error); if (!layer) { - Nan::ThrowTypeError(layer.error().message.c_str()); + Nan::ThrowTypeError(error.message.c_str()); return; } @@ -774,9 +776,10 @@ void NodeMap::SetFilter(const Nan::FunctionCallbackInfo& info) { Filter filter; if (!info[1]->IsNull() && !info[1]->IsUndefined()) { - Result converted = convert(info[1]); + Error error; + mbgl::optional converted = convert(info[1], error); if (!converted) { - Nan::ThrowTypeError(converted.error().message.c_str()); + Nan::ThrowTypeError(error.message.c_str()); return; } filter = std::move(*converted); @@ -908,23 +911,24 @@ void NodeMap::QueryRenderedFeatures(const Nan::FunctionCallbackInfo& //Check if filter is provided. If set it must be a valid Filter object if (Nan::Has(options, Nan::New("filter").ToLocalChecked()).FromJust()) { auto filterOption = Nan::Get(options, Nan::New("filter").ToLocalChecked()).ToLocalChecked(); - Result converted = convert(filterOption); + Error error; + mbgl::optional converted = convert(filterOption, error); if (!converted) { - return Nan::ThrowTypeError(converted.error().message.c_str()); + return Nan::ThrowTypeError(error.message.c_str()); } queryOptions.filter = std::move(*converted); } } try { - std::vector result; + std::vector optional; if (Nan::Get(posOrBox, 0).ToLocalChecked()->IsArray()) { auto pos0 = Nan::Get(posOrBox, 0).ToLocalChecked().As(); auto pos1 = Nan::Get(posOrBox, 1).ToLocalChecked().As(); - result = nodeMap->map->queryRenderedFeatures(mbgl::ScreenBox { + optional = nodeMap->map->queryRenderedFeatures(mbgl::ScreenBox { { Nan::Get(pos0, 0).ToLocalChecked()->NumberValue(), Nan::Get(pos0, 1).ToLocalChecked()->NumberValue() @@ -935,15 +939,15 @@ void NodeMap::QueryRenderedFeatures(const Nan::FunctionCallbackInfo& }, queryOptions); } else { - result = nodeMap->map->queryRenderedFeatures(mbgl::ScreenCoordinate { + optional = nodeMap->map->queryRenderedFeatures(mbgl::ScreenCoordinate { Nan::Get(posOrBox, 0).ToLocalChecked()->NumberValue(), Nan::Get(posOrBox, 1).ToLocalChecked()->NumberValue() }, queryOptions); } auto array = Nan::New(); - for (unsigned int i = 0; i < result.size(); i++) { - array->Set(i, toJS(result[i])); + for (unsigned int i = 0; i < optional.size(); i++) { + array->Set(i, toJS(optional[i])); } info.GetReturnValue().Set(array); } catch (const std::exception &ex) { diff --git a/platform/qt/src/qmapboxgl.cpp b/platform/qt/src/qmapboxgl.cpp index b1c0a11ee..3d5da10c3 100644 --- a/platform/qt/src/qmapboxgl.cpp +++ b/platform/qt/src/qmapboxgl.cpp @@ -1250,9 +1250,10 @@ void QMapboxGL::addSource(const QString &id, const QVariantMap ¶ms) using namespace mbgl::style; using namespace mbgl::style::conversion; - Result> source = convert>(QVariant(params), id.toStdString()); + Error error; + mbgl::optional> source = convert>(QVariant(params), error, id.toStdString()); if (!source) { - qWarning() << "Unable to add source:" << source.error().message.c_str(); + qWarning() << "Unable to add source:" << error.message.c_str(); return; } @@ -1291,7 +1292,8 @@ void QMapboxGL::updateSource(const QString &id, const QVariantMap ¶ms) } if (params.contains("data")) { - auto result = convertGeoJSON(params["data"]); + Error error; + auto result = convertGeoJSON(params["data"], error); if (result) { sourceGeoJSON->setGeoJSON(*result); } @@ -1362,9 +1364,10 @@ void QMapboxGL::addLayer(const QVariantMap ¶ms) using namespace mbgl::style; using namespace mbgl::style::conversion; - Result> layer = convert>(QVariant(params)); + Error error; + mbgl::optional> layer = convert>(QVariant(params), error); if (!layer) { - qWarning() << "Unable to add layer:" << layer.error().message.c_str(); + qWarning() << "Unable to add layer:" << error.message.c_str(); return; } @@ -1445,9 +1448,10 @@ void QMapboxGL::setFilter(const QString& layer, const QVariant& filter) Filter filter_; - Result converted = convert(filter); + Error error; + mbgl::optional converted = convert(filter, error); if (!converted) { - qWarning() << "Error parsing filter:" << converted.error().message.c_str(); + qWarning() << "Error parsing filter:" << error.message.c_str(); return; } filter_ = std::move(*converted); diff --git a/platform/qt/src/qt_geojson.hpp b/platform/qt/src/qt_geojson.hpp index 038b05194..7c50c663d 100644 --- a/platform/qt/src/qt_geojson.hpp +++ b/platform/qt/src/qt_geojson.hpp @@ -180,20 +180,21 @@ namespace style { namespace conversion { template <> -Result convertGeoJSON(const QMapbox::Feature& feature) { - return Result { GeoJSON { asMapboxGLFeature(feature) } }; +optional convertGeoJSON(const QMapbox::Feature& feature, Error&) { + return GeoJSON { asMapboxGLFeature(feature) }; } template <> -Result convertGeoJSON(const QVariant& value) { +optional convertGeoJSON(const QVariant& value, Error& error) { #if QT_VERSION >= 0x050000 if (value.typeName() == QStringLiteral("QMapbox::Feature")) { #else if (value.typeName() == QString("QMapbox::Feature")) { #endif - return convertGeoJSON(value.value()); + return convertGeoJSON(value.value(), error); } else if (value.type() != QVariant::ByteArray) { - return Error { "JSON data must be in QByteArray" }; + error = { "JSON data must be in QByteArray" }; + return {}; } auto data = value.toByteArray(); @@ -208,13 +209,14 @@ Result convertGeoJSON(const QVariant& value) { if (d.HasParseError()) { std::stringstream message; message << d.GetErrorOffset() << " - " << rapidjson::GetParseError_En(d.GetParseError()); - - return Error { message.str() }; + error = { message.str() }; + return {}; } - Result geoJSON = convertGeoJSON(d); + optional geoJSON = convertGeoJSON(d, error); if (!geoJSON) { - return Error { geoJSON.error().message }; + error = { error.message }; + return {}; } return geoJSON; diff --git a/src/mbgl/style/parser.cpp b/src/mbgl/style/parser.cpp index 926c24373..d970d8bbc 100644 --- a/src/mbgl/style/parser.cpp +++ b/src/mbgl/style/parser.cpp @@ -116,10 +116,11 @@ void Parser::parseSources(const JSValue& value) { for (const auto& property : value.GetObject()) { std::string id = *conversion::toString(property.name); - conversion::Result> source = - conversion::convert>(property.value, id); + conversion::Error error; + optional> source = + conversion::convert>(property.value, error, id); if (!source) { - Log::Warning(Event::ParseStyle, source.error().message); + Log::Warning(Event::ParseStyle, error.message); continue; } @@ -222,9 +223,10 @@ void Parser::parseLayer(const std::string& id, const JSValue& value, std::unique layer = reference->baseImpl->cloneRef(id); conversion::setPaintProperties(*layer, value); } else { - conversion::Result> converted = conversion::convert>(value); + conversion::Error error; + optional> converted = conversion::convert>(value, error); if (!converted) { - Log::Warning(Event::ParseStyle, converted.error().message); + Log::Warning(Event::ParseStyle, error.message); return; } layer = std::move(*converted); @@ -232,20 +234,20 @@ void Parser::parseLayer(const std::string& id, const JSValue& value, std::unique } std::vector Parser::fontStacks() const { - std::set result; + std::set optional; for (const auto& layer : layers) { if (layer->is()) { PropertyValue textFont = layer->as()->getTextFont(); if (textFont.isUndefined()) { - result.insert({"Open Sans Regular", "Arial Unicode MS Regular"}); + optional.insert({"Open Sans Regular", "Arial Unicode MS Regular"}); } else if (textFont.isConstant()) { - result.insert(textFont.asConstant()); + optional.insert(textFont.asConstant()); } else if (textFont.isCameraFunction()) { textFont.asCameraFunction().stops.match( [&] (const auto& stops) { for (const auto& stop : stops.stops) { - result.insert(stop.second); + optional.insert(stop.second); } } ); @@ -253,7 +255,7 @@ std::vector Parser::fontStacks() const { } } - return std::vector(result.begin(), result.end()); + return std::vector(optional.begin(), optional.end()); } } // namespace style diff --git a/src/mbgl/style/property_parsing.cpp b/src/mbgl/style/property_parsing.cpp deleted file mode 100644 index 16ce0f4ad..000000000 --- a/src/mbgl/style/property_parsing.cpp +++ /dev/null @@ -1,29 +0,0 @@ -#include - -namespace mbgl { -namespace style { - -optional parseTransitionOptions(const char *, const JSValue& value) { - if (!value.IsObject()) { - return {}; - } - - optional duration; - if (value.HasMember("duration") && value["duration"].IsNumber()) { - duration.emplace(Milliseconds(value["duration"].GetUint())); - } - - optional delay; - if (value.HasMember("delay") && value["delay"].IsNumber()) { - delay.emplace(Milliseconds(value["delay"].GetUint())); - } - - if (!duration && !delay) { - return {}; - } - - return TransitionOptions { duration, delay }; -} - -} // namespace style -} // namespace mbgl diff --git a/src/mbgl/style/property_parsing.hpp b/src/mbgl/style/property_parsing.hpp deleted file mode 100644 index b542c8ae4..000000000 --- a/src/mbgl/style/property_parsing.hpp +++ /dev/null @@ -1,26 +0,0 @@ -#pragma once - -#include -#include -#include -#include - -#include - -namespace mbgl { -namespace style { - -template -PropertyValue parseProperty(const char* name, const JSValue& value) { - conversion::Result> result = conversion::convert>(value); - if (!result) { - Log::Warning(Event::ParseStyle, "%s: %s", name, result.error().message); - return {}; - } - return *result; -} - -optional parseTransitionOptions(const char * name, const JSValue&); - -} // namespace style -} // namespace mbgl diff --git a/src/mbgl/style/sources/geojson_source_impl.cpp b/src/mbgl/style/sources/geojson_source_impl.cpp index b050e5482..6431d5faa 100644 --- a/src/mbgl/style/sources/geojson_source_impl.cpp +++ b/src/mbgl/style/sources/geojson_source_impl.cpp @@ -21,11 +21,12 @@ namespace style { namespace conversion { template <> -Result convertGeoJSON(const JSValue& value) { +optional convertGeoJSON(const JSValue& value, Error& error) { try { return mapbox::geojson::convert(value); } catch (const std::exception& ex) { - return Error{ ex.what() }; + error = { ex.what() }; + return {}; } } } // namespace conversion @@ -136,10 +137,11 @@ void GeoJSONSource::Impl::loadDescription(FileSource& fileSource) { invalidateTiles(); - conversion::Result geoJSON = conversion::convertGeoJSON(d); + conversion::Error error; + optional geoJSON = conversion::convertGeoJSON(d, error); if (!geoJSON) { Log::Error(Event::ParseStyle, "Failed to parse GeoJSON data: %s", - geoJSON.error().message.c_str()); + error.message.c_str()); // Create an empty GeoJSON VT object to make sure we're not infinitely waiting for // tiles to load. _setGeoJSON(GeoJSON{ FeatureCollection{} }); diff --git a/src/mbgl/style/tile_source_impl.cpp b/src/mbgl/style/tile_source_impl.cpp index daa6389a8..9ea596d38 100644 --- a/src/mbgl/style/tile_source_impl.cpp +++ b/src/mbgl/style/tile_source_impl.cpp @@ -24,9 +24,10 @@ Tileset TileSourceImpl::parseTileJSON(const std::string& json, const std::string throw std::runtime_error(message.str()); } - conversion::Result result = conversion::convert(document); + conversion::Error error; + optional result = conversion::convert(document, error); if (!result) { - throw std::runtime_error(result.error().message); + throw std::runtime_error(error.message); } // TODO: Remove this hack by delivering proper URLs in the TileJSON to begin with. diff --git a/test/style/conversion/function.test.cpp b/test/style/conversion/function.test.cpp index 4dc6549c7..08637d40c 100644 --- a/test/style/conversion/function.test.cpp +++ b/test/style/conversion/function.test.cpp @@ -10,45 +10,47 @@ using namespace mbgl; using namespace mbgl::style; using namespace mbgl::style::conversion; -auto parseFunction(const std::string& src) { - JSDocument doc; - doc.Parse<0>(src); - return convert, JSValue>(doc); -} - TEST(StyleConversion, Function) { + Error error; + + auto parseFunction = [&](const std::string& src) { + JSDocument doc; + doc.Parse<0>(src); + return convert, JSValue>(doc, error); + }; + auto fn1 = parseFunction("{\"stops\":[]}"); ASSERT_FALSE(fn1); - ASSERT_EQ("function must have at least one stop", fn1.error().message); + ASSERT_EQ("function must have at least one stop", error.message); auto fn2 = parseFunction("{\"stops\":[1]}"); ASSERT_FALSE(fn2); - ASSERT_EQ("function stop must be an array", fn2.error().message); + ASSERT_EQ("function stop must be an array", error.message); auto fn3 = parseFunction("{\"stops\":[[]]}"); ASSERT_FALSE(fn3); - ASSERT_EQ("function stop must have two elements", fn3.error().message); + ASSERT_EQ("function stop must have two elements", error.message); auto fn4 = parseFunction("{\"stops\":[[-1,-1]]}"); ASSERT_TRUE(bool(fn4)); auto fn5 = parseFunction("{\"stops\":[[0,1,2]]}"); ASSERT_FALSE(fn5); - ASSERT_EQ("function stop must have two elements", fn5.error().message); + ASSERT_EQ("function stop must have two elements", error.message); auto fn6 = parseFunction("{\"stops\":[[0,\"x\"]]}"); ASSERT_FALSE(fn6); - ASSERT_EQ("value must be a number", fn6.error().message); + ASSERT_EQ("value must be a number", error.message); auto fn7 = parseFunction("{}"); ASSERT_FALSE(fn7); - ASSERT_EQ("function value must specify stops", fn7.error().message); + ASSERT_EQ("function value must specify stops", error.message); auto fn8 = parseFunction("[]"); ASSERT_FALSE(fn8); - ASSERT_EQ("function must be an object", fn8.error().message); + ASSERT_EQ("function must be an object", error.message); auto fn9 = parseFunction("{\"stops\":[[0,0]],\"base\":false}"); ASSERT_FALSE(fn9); - ASSERT_EQ("function base must be a number", fn9.error().message); + ASSERT_EQ("function base must be a number", error.message); } diff --git a/test/style/conversion/geojson_options.test.cpp b/test/style/conversion/geojson_options.test.cpp index ddf261ea5..e6bd984f3 100644 --- a/test/style/conversion/geojson_options.test.cpp +++ b/test/style/conversion/geojson_options.test.cpp @@ -12,21 +12,24 @@ using namespace mbgl::style::conversion; TEST(GeoJSONOptions, Basic) { ValueMap map; Value raw(map); - Result converted = convert(raw); + Error error; + mbgl::optional converted = convert(raw, error); ASSERT_TRUE((bool) converted); } TEST(GeoJSONOptions, ErrorHandling) { ValueMap map {{"maxzoom", std::string{"should not be a string"}}}; Value raw(map); - Result converted = convert(raw); + Error error; + mbgl::optional converted = convert(raw, error); ASSERT_FALSE((bool) converted); } TEST(GeoJSONOptions, RetainsDefaults) { ValueMap map; Value raw(map); - GeoJSONOptions converted = *convert(raw); + Error error; + GeoJSONOptions converted = *convert(raw, error); GeoJSONOptions defaults; // GeoJSON-VT @@ -54,7 +57,8 @@ TEST(GeoJSONOptions, FullConversion) { {"clusterMaxZoom", 5.0f} }; Value raw(map); - GeoJSONOptions converted = *convert(raw); + Error error; + GeoJSONOptions converted = *convert(raw, error); // GeoJSON-VT ASSERT_EQ(converted.maxzoom, 1); diff --git a/test/style/conversion/layer.test.cpp b/test/style/conversion/layer.test.cpp index b27c1841e..cfe2662fb 100644 --- a/test/style/conversion/layer.test.cpp +++ b/test/style/conversion/layer.test.cpp @@ -11,10 +11,11 @@ using namespace mbgl::style; using namespace mbgl::style::conversion; using namespace std::literals::chrono_literals; -auto parseLayer(const std::string& src) { +std::unique_ptr parseLayer(const std::string& src) { JSDocument doc; doc.Parse<0>(src); - return convert, JSValue>(doc); + Error error; + return std::move(*convert, JSValue>(doc, error)); } TEST(StyleConversion, LayerTransition) { @@ -34,13 +35,13 @@ TEST(StyleConversion, LayerTransition) { } })JSON"); - ASSERT_EQ(400ms, *(*layer)->as()->impl->paint.cascading + ASSERT_EQ(400ms, *layer->as()->impl->paint.cascading .get().getTransition({}).duration); - ASSERT_EQ(500ms, *(*layer)->as()->impl->paint.cascading + ASSERT_EQ(500ms, *layer->as()->impl->paint.cascading .get().getTransition({}).delay); - ASSERT_EQ(100ms, *(*layer)->as()->impl->paint.cascading + ASSERT_EQ(100ms, *layer->as()->impl->paint.cascading .get().getTransition({"class"}).duration); - ASSERT_FALSE(bool((*layer)->as()->impl->paint.cascading + ASSERT_FALSE(bool(layer->as()->impl->paint.cascading .get().getTransition({"class"}).delay)); } diff --git a/test/style/filter.test.cpp b/test/style/filter.test.cpp index 2f1224607..c70792d8e 100644 --- a/test/style/filter.test.cpp +++ b/test/style/filter.test.cpp @@ -16,7 +16,10 @@ using namespace mbgl::style; Filter parse(const char * expression) { rapidjson::GenericDocument, rapidjson::CrtAllocator> doc; doc.Parse<0>(expression); - return *conversion::convert(doc); + conversion::Error error; + optional filter = conversion::convert(doc, error); + EXPECT_TRUE(bool(filter)); + return *filter; } Feature feature(const PropertyMap& properties, const Geometry& geometry = Point()) {