Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[ios, macos] Fix number conversions when expressions cast to large numbers. #13580

Merged
merged 2 commits into from
Dec 14, 2018

Conversation

fabian-guerra
Copy link
Contributor

Fixes #13527

@fabian-guerra fabian-guerra added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS crash expressions labels Dec 14, 2018
@fabian-guerra fabian-guerra added this to the release-iowaska milestone Dec 14, 2018
@fabian-guerra fabian-guerra self-assigned this Dec 14, 2018
@fabian-guerra fabian-guerra requested a review from 1ec5 as a code owner December 14, 2018 00:48
@fabian-guerra fabian-guerra requested a review from a team December 14, 2018 00:48
@@ -85,7 +85,7 @@ class ConversionTraits<Holder> {
static optional<float> toNumber(const Holder& holder) {
const id value = holder.value;
if (_isNumber(value)) {
return ((NSNumber *)value).floatValue;
return ((NSNumber *)value).doubleValue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return type is an optional<float> here. I think we should check that value is in the range of a float. Do we have an existing method for that?

@fabian-guerra fabian-guerra force-pushed the fabian-conversion-13527 branch from 5984fde to af48aab Compare December 14, 2018 18:48
Copy link
Contributor

@julianrex julianrex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fabian-guerra fabian-guerra merged commit fed201a into master Dec 14, 2018
@fabian-guerra fabian-guerra deleted the fabian-conversion-13527 branch December 14, 2018 22:40
@1ec5 1ec5 modified the milestones: release-iowaska, release-java Dec 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
crash expressions iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants