-
Notifications
You must be signed in to change notification settings - Fork 10
Added JNumber conversions #38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
7db4283
8c5b1be
065fcad
403d9b3
795838c
171f30d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
package scalajson.ast | ||
|
||
import org.scalameter.{Bench, Gen} | ||
|
||
object JNumberConversionBenchmark extends Bench.ForkedTime { | ||
def intString: Gen[String] = | ||
for { | ||
size <- Gen.range("seed")(300000, 1500000, 300000) | ||
} yield { | ||
size.toString | ||
} | ||
|
||
def floatString: Gen[String] = | ||
for { | ||
a <- Gen.range("seed")(300000, 1500000, 300000) | ||
b <- Gen.range("seed")(300000, 1500000, 300000) | ||
} yield { | ||
s"$a.$b".toFloat.toString | ||
} | ||
|
||
private val intConstructedFlag = NumberFlags.intConstructed | ||
private val floatConstructedFlag = NumberFlags.floatConstructed | ||
|
||
performance of "intBitFlagCheckSuccess" in { | ||
using(intString) in { value: String => | ||
if ((intConstructedFlag & NumberFlags.int) == NumberFlags.int) | ||
Some(value.toInt) | ||
else { | ||
try { | ||
val asInt = value.toInt | ||
if (BigInt(value) == BigInt(asInt)) | ||
Some(asInt) | ||
else | ||
None | ||
} catch { | ||
case _: NumberFormatException => None | ||
} | ||
} | ||
} | ||
} | ||
|
||
performance of "intManualCheckSuccess" in { | ||
using(intString) in { value: String => | ||
try { | ||
val asInt = value.toInt | ||
if (BigInt(value) == BigInt(asInt)) | ||
Some(asInt) | ||
else | ||
None | ||
} catch { | ||
case _: NumberFormatException => None | ||
} | ||
} | ||
} | ||
|
||
performance of "floatBitFlagCheckSuccess" in { | ||
using(floatString) in { value: String => | ||
if ((floatConstructedFlag & NumberFlags.float) == NumberFlags.float) | ||
Some(value.toFloat) | ||
else { | ||
try { | ||
val asFloat = value.toFloat | ||
if (BigDecimal(value) == BigDecimal(asFloat.toDouble)) | ||
Some(asFloat) | ||
else | ||
None | ||
} catch { | ||
case _: NumberFormatException => None | ||
} | ||
|
||
} | ||
} | ||
} | ||
|
||
performance of "floatManualCheckSuccess" in { | ||
using(floatString) in { value: String => | ||
try { | ||
val asFloat = value.toFloat | ||
if (BigDecimal(value) == BigDecimal(asFloat.toDouble)) | ||
Some(asFloat) | ||
else | ||
None | ||
} catch { | ||
case _: NumberFormatException => None | ||
} | ||
|
||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,15 +50,20 @@ final case class JString(value: String) extends JValue { | |
} | ||
|
||
object JNumber { | ||
def apply(value: Int): JNumber = new JNumber(value.toString) | ||
def apply(value: Int): JNumber = | ||
new JNumber(value.toString)(NumberFlags.intConstructed) | ||
|
||
def apply(value: Integer): JNumber = new JNumber(value.toString) | ||
def apply(value: Integer): JNumber = | ||
new JNumber(value.toString)(NumberFlags.intConstructed) | ||
|
||
def apply(value: Long): JNumber = new JNumber(value.toString) | ||
def apply(value: Long): JNumber = | ||
new JNumber(value.toString)(NumberFlags.longConstructed) | ||
|
||
def apply(value: BigInt): JNumber = new JNumber(value.toString()) | ||
def apply(value: BigInt): JNumber = | ||
new JNumber(value.toString())(NumberFlags.bigIntConstructed) | ||
|
||
def apply(value: BigDecimal): JNumber = new JNumber(value.toString()) | ||
def apply(value: BigDecimal): JNumber = | ||
new JNumber(value.toString())(NumberFlags.bigDecimalConstructed) | ||
|
||
/** | ||
* @param value | ||
|
@@ -67,7 +72,7 @@ object JNumber { | |
def apply(value: Double): JValue = value match { | ||
case n if n.isNaN => JNull | ||
case n if n.isInfinity => JNull | ||
case _ => new JNumber(value.toString) | ||
case _ => new JNumber(value.toString)(NumberFlags.doubleConstructed) | ||
} | ||
|
||
/** | ||
|
@@ -77,12 +82,12 @@ object JNumber { | |
def apply(value: Float): JValue = value match { | ||
case n if java.lang.Float.isNaN(n) => JNull | ||
case n if n.isInfinity => JNull | ||
case _ => new JNumber(value.toString) | ||
case _ => new JNumber(value.toString)(NumberFlags.floatConstructed) | ||
} | ||
|
||
def fromString(value: String): Option[JNumber] = | ||
value match { | ||
case jNumberRegex(_ *) => Some(new JNumber(value)) | ||
case jNumberRegex(_ *) => Some(new JNumber(value)(0)) | ||
case _ => None | ||
} | ||
|
||
|
@@ -97,15 +102,9 @@ object JNumber { | |
*/ | ||
// Due to a restriction in Scala 2.10, we cant override/replace the default apply method | ||
// generated by the compiler even when the constructor itself is marked private | ||
final class JNumber private[ast] (val value: String) extends JValue { | ||
|
||
/** | ||
* Javascript specification for numbers specify a [[scala.Double]], so this is the default export method to `Javascript` | ||
* | ||
* @param value | ||
*/ | ||
def this(value: Double) = this(value.toString) | ||
|
||
final class JNumber private[ast] (val value: String)( | ||
private[ast] val constructedFlag: Int) | ||
extends JValue { | ||
override def toUnsafe: unsafe.JValue = unsafe.JNumber(value) | ||
|
||
override def toJsAny: js.Any = value.toDouble match { | ||
|
@@ -143,9 +142,85 @@ final class JNumber private[ast] (val value: String) extends JValue { | |
|
||
def copy(value: String): JNumber = | ||
value match { | ||
case jNumberRegex(_ *) => new JNumber(value) | ||
case jNumberRegex(_ *) => new JNumber(value)(0) | ||
case _ => throw new NumberFormatException(value) | ||
} | ||
|
||
def toInt: Option[Long] = { | ||
if ((constructedFlag & NumberFlags.int) == NumberFlags.int) | ||
Some(value.toInt) | ||
else { | ||
try { | ||
val asInt = value.toInt | ||
if (BigInt(value) == BigInt(asInt)) | ||
Some(asInt) | ||
else | ||
None | ||
} catch { | ||
case _: NumberFormatException => None | ||
} | ||
} | ||
} | ||
|
||
def toLong: Option[Long] = { | ||
if ((constructedFlag & NumberFlags.long) == NumberFlags.long) | ||
Some(value.toLong) | ||
else { | ||
try { | ||
val asLong = value.toLong | ||
if (BigInt(value) == BigInt(asLong)) | ||
Some(asLong) | ||
else | ||
None | ||
} catch { | ||
case _: NumberFormatException => None | ||
} | ||
} | ||
} | ||
|
||
def toBigInt: Option[BigInt] = { | ||
if ((constructedFlag & NumberFlags.bigInt) == NumberFlags.bigInt) | ||
Some(BigInt(value)) | ||
else { | ||
try { | ||
Some(BigInt(value)) | ||
} catch { | ||
case _: NumberFormatException => None | ||
} | ||
} | ||
} | ||
|
||
def toBigDecimal: Option[BigDecimal] = { | ||
try { | ||
Some(BigDecimal(value)) | ||
} catch { | ||
case _: NumberFormatException => None | ||
} | ||
} | ||
|
||
def toFloat: Option[Float] = { | ||
if ((constructedFlag & NumberFlags.float) == NumberFlags.float) | ||
Some(value.toFloat) | ||
else { | ||
val asFloat = value.toFloat | ||
if (BigDecimal(value) == BigDecimal(asFloat.toDouble)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Scala's
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TBH, I wasn't entirely sure how floats worked in Scala, but after reading this I think I need to do some more research If I have a better way of detecting losss of precision To answer your questions
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Spray used
If we care about correctness so much, my vote would be get rid of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good, ill have a look to get rid of it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Getting rid of
So neither having the exact decimal representation of the double value nor having the best rounding of that value really represents what's going on. Instead, if you overflow (get positive or negative infinity) that's wrong; and if you underflow (the number is not zero but you return zero) that is also wrong. Otherwise, the parse should just return the best representation of the value, and everyone should keep in mind (just like always) The reason to get rid of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Both If we are letting them survive, trusting that the user knows what they are doing, I think the Scala idiomatic way is to behave like StringOps and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @eed3si9n - |
||
Some(asFloat) | ||
else | ||
None | ||
} | ||
} | ||
|
||
def toDouble: Option[Double] = { | ||
if ((constructedFlag & NumberFlags.double) == NumberFlags.double) | ||
Some(value.toDouble) | ||
else { | ||
val asDouble = value.toDouble | ||
if (BigDecimal(value) == BigDecimal(asDouble)) | ||
Some(asDouble) | ||
else | ||
None | ||
} | ||
} | ||
} | ||
|
||
/** Represents a JSON Boolean value, which can either be a | ||
|
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could
constructedFlag
be a smell that we are doing poor man's object orientation?If we want lossless (and performant?) round trip from Double -> JNumber -> Double, and Json -> AST -> Json, would it make sense to split this class into
private[ast] JInt
,JLong
,JFloat
,JDouble
,JBigInt
,JBigDecimial
, andJStringNumber
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I didn't realise this at the time, @sirthias what do you think of this? If we are going to do performant number conversions may as well represent the cases as various classes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's some pretty wide fanout. In principle if encoding and decoding in memory is a likely scenario, this is the way to do it, but I'm skeptical that this is an important use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's #41 that splits
JNumber
up.