RDART-866: kn/decimal128 web support #1713
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nielsenko
force-pushed
the
kn/minimal-web-support
branch
from
4d9f2ce to
1e8e180
Compare
nielsenko
force-pushed
the
kn/decimal128-web-support
branch
from
bdc06b8 to
8ca922b
Compare
Pull Request Test Coverage Report for Build 9407798276Details
💛 - Coveralls |
nirinchev
reviewed
Jun 6, 2024
|
|
||
| /// Converts a `double` into a [Decimal128]. | ||
| factory Decimal128.fromDouble(double value) => webNotSupported(); | ||
| factory Decimal128.fromDouble(double value) { |
There was a problem hiding this comment.
Can we take a shortcut here and do a Decimal.parse(double.toString())?
Comment on lines
+116
to
+98
| final sign = _value.compareTo(other._value); | ||
| if (sign < 0) return -1; | ||
| if (sign > 0) return 1; | ||
| return 0; |
There was a problem hiding this comment.
Any reason not to return sign directly?
There was a problem hiding this comment.
Yes, I'm trying to make as many as the decimal128_test.dart tests pass as possible. The behaviour of double will not just return the value of a - b, hence native.Decimal128 doesn't either, and nor should web.Decimal128.
But you could argue that we are already deviating a lot from the semantics of IEEE 754-2008, and the semantics of double so perhaps it is a mood point.
| import 'package:decimal/decimal.dart'; | ||
| import 'package:rational/rational.dart'; | ||
|
|
||
| import 'package:realm_dart/src/handles/native/convert.dart'; |
There was a problem hiding this comment.
Should this depend on something in native?
nielsenko
force-pushed
the
kn/decimal128-web-support
branch
from
8ca922b to
8dcc430
Compare
Pull Request Test Coverage Report for Build 9413918326Details
💛 - Coveralls |
nielsenko
force-pushed
the
kn/decimal128-web-support
branch
from
8dcc430 to
e5d73af
Compare
Pull Request Test Coverage Report for Build 9414419493Details
💛 - Coveralls |
nielsenko
force-pushed
the
kn/decimal128-web-support
branch
from
e5d73af to
04cb086
Compare
Pull Request Test Coverage Report for Build 9415009835Details
💛 - Coveralls |
nielsenko
changed the title
nirinchev
approved these changes
Jun 7, 2024
nirinchev
added a commit
that referenced
this pull request
Jun 12, 2024
* main: Add vNext Changelog header (#1717) [Release 3.0.0] (#1716) libraryVersion moved to realm_libary.dart (take 2) libraryVersion moved to realm_libary.dart Update CHANGELOG (#1715) Github composite action for setting up flutter on runner (#1710) RDART-866: kn/decimal128 web support (#1713) Reduce expected gain of memEquals for test stability Refresh after awaiting download to stabilize tests RDART-866: Minimal web support (#1699) RDART-1052: Update realm-core to v14.9.0 (#1704) RDART-1020: Fix writeAsync behaviour (#1666) RDART-999: Fix flutter test dlopen (#1623) RDART-1045: Expose setrlimit ios (#1700) RDART-962: Use xcode 15.4 (#1548) RDART-1039: Drop catalyst support. Flutter doesn't support it (#1696) # Conflicts: # packages/realm_dart/src/realm-core
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As part addressing #1374 in it was found that not allowing
Decimal128fields on unmanaged realm models when used on the web platform was problematic for one of our users. Hence this PR adds support for that as well.This is not a fully compliant IEEE 754-2008 Decimal128 implementation, as it is just based on the decimal package. Which is based on the rational package, which is again based on the BigInt class.
The issues are mostly in some of the odd corner cases of IEEE 754-2008 floating point values, such as:
Currently anything involving
Decimal128.nan,Decimal128.infinity, andDecimal128.negativeInfinitywill throw on web, includingDecimal128.isNaN.Also, be warned that this class is incredible slow compared to the native implementation, especially division.