Closed mdedetrich closed 6 years ago
Merging #38 into master will decrease coverage by
9.25%
. The diff coverage is17.58%
.
@@ Coverage Diff @@
## master #38 +/- ##
==========================================
- Coverage 44.68% 35.43% -9.26%
==========================================
Files 5 5
Lines 687 906 +219
Branches 133 194 +61
==========================================
+ Hits 307 321 +14
- Misses 380 585 +205
Impacted Files | Coverage Δ | |
---|---|---|
...s/src/main/scala/scalajson/ast/unsafe/JValue.scala | 0% <0%> (ø) |
:arrow_up: |
js/src/main/scala/scalajson/ast/JValue.scala | 0% <0%> (ø) |
:arrow_up: |
shared/src/main/scala/scalajson/ast/package.scala | 59.91% <100%> (+2.13%) |
:arrow_up: |
...m/src/main/scala/scalajson/ast/unsafe/JValue.scala | 42.79% <22.97%> (-16.72%) |
:arrow_down: |
jvm/src/main/scala/scalajson/ast/JValue.scala | 47.36% <25.67%> (-26.03%) |
:arrow_down: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update fd5637b...171f30d. Read the comment docs.
This is an implementation which adds various conversions from
JNumber
to other number types (see https://github.com/mdedetrich/scalajson/issues/37). Unlike the previous implementation, these conversionstoDouble
will returnNone
if the value inside theJNumber
is too big and you will lose data)To make the various number converter calls as performant as possible, when you construct a
JNumber
we store a bitFlag (calledconstructedFlag
) which tracks how aJNumber
is constructed. This means that if you construct aJNumber
as a double with its argument (i.e.JNumber(3446437343d)
), we first do a check against this bitflag (which is a single if statement) and immediately return aSome(value.toDouble)
(since we know its a double), otherwise we do a more expensive check. This also means that theJNumber
has an extra int per instance ofJNumber
in terms of memory (so we are trading a very small amount of extra memory for a more performant number conversion in typical use cases).The checks were inspired from argonaut (i.e. https://github.com/argonaut-io/argonaut/blob/master/argonaut/shared/src/main/scala/argonaut/JsonNumber.scala) afaik, these are known to be correct
One note, due to how
ast.unsafe.JNumber
was constructed (theString
constructer by default is public), we now added the a second parameter to the constructor list which is not hidden (i.e. itsfinal case class JNumber(value: String, constructedFlag: Int = 0)
) which means theunapply
method has been changed. Although this change is breaking, it means theunsafe
AST does give you control over specifying theconstructedFlag
.To be done
Here is the current results for the benchmarks
@ktoso @travisbrown @Ichoran @eed3si9n @sirthias Thoughts?