mbeckerle commented on a change in pull request #571:
URL: https://github.com/apache/daffodil/pull/571#discussion_r636125964



##########
File path: daffodil-lib/src/test/scala/passera/test/TestULong.scala
##########
@@ -64,5 +63,45 @@ class TestULong {
     val remainder = mm1 % mm2
     assertEquals(ULong(1), remainder)
   }
-  
+
+  @Test def testULongMostNegativeLong1: Unit = {
+    val v = ULong(Long.MinValue)
+    val vbi = v.toBigInt
+    val vhex = vbi.toString(16)
+    assertEquals("8000000000000000", vhex)
+    assertEquals("8000000000000000", v.toHexString)
+    assertEquals(-9223372036854775808L, v.longValue)
+    assertEquals("9223372036854775808", v.toString)
+  }
+
+  @Test def testULongMaxValue: Unit = {
+    val v = ULong.MaxValue
+    assertEquals("FFFFFFFFFFFFFFFF", v.toHexString.toUpperCase)
+    assertEquals(ULong(0), v + ULong(1))
+    val v1 = ULong.MaxValue.toBigInt.add(JBigInt.TWO) // 0x8000000000000001
+    assertEquals(ULong(1), ULong(v1)) // preserves only 64 bits
+  }
+
+  @Test def testULongFromBigInt: Unit = {
+    val zero = JBigInt.ZERO
+    val one = JBigInt.ONE
+    val two = JBigInt.TWO
+    val minusOne = JBigInt.valueOf(-1)
+    val minusTwo = JBigInt.valueOf(-2)
+    assertEquals(0, ULong(zero).toInt)
+    assertEquals(1, ULong(one).toInt)
+    assertEquals(2, ULong(two).toInt)
+    assertEquals(-1, ULong(minusOne).toInt)
+    assertEquals(-2, ULong(minusTwo).toInt)
+    assertEquals("7FFFFFFFFFFFFFFF", (ULong(Long.MinValue) - 
ULong(1)).toHexString.toUpperCase)
+  }
+
+  @Test def testULongShift: Unit = {
+    // NO sign extension since a ULong has no sign bit.
+    assertEquals("7FFFFFFFFFFFFFFF", (ULong.MaxValue >> 
1).toHexString.toUpperCase)
+    assertEquals("7FFFFFFFFFFFFFFF", (ULong.MaxValue >>> 
1).toHexString.toUpperCase)
+    assertEquals(1, (ULong.MaxValue >> 63).toInt)
+    assertEquals(ULong.MaxValue, ULong.MaxValue >> 64)

Review comment:
       Yeah, they are just taking the shift count modulo width. I.e., (count & 
0x3F) in this case, 1F for Int, F for short, 7 for byte. 
   
   I am inclined to follow their style, so as to not fully fork this library, 
but be able to propagate our changes here back to the original authors. 

##########
File path: daffodil-lib/src/test/scala/passera/test/TestULong.scala
##########
@@ -64,5 +63,45 @@ class TestULong {
     val remainder = mm1 % mm2
     assertEquals(ULong(1), remainder)
   }
-  
+
+  @Test def testULongMostNegativeLong1: Unit = {
+    val v = ULong(Long.MinValue)
+    val vbi = v.toBigInt
+    val vhex = vbi.toString(16)
+    assertEquals("8000000000000000", vhex)
+    assertEquals("8000000000000000", v.toHexString)
+    assertEquals(-9223372036854775808L, v.longValue)
+    assertEquals("9223372036854775808", v.toString)
+  }
+
+  @Test def testULongMaxValue: Unit = {
+    val v = ULong.MaxValue
+    assertEquals("FFFFFFFFFFFFFFFF", v.toHexString.toUpperCase)
+    assertEquals(ULong(0), v + ULong(1))
+    val v1 = ULong.MaxValue.toBigInt.add(JBigInt.TWO) // 0x8000000000000001
+    assertEquals(ULong(1), ULong(v1)) // preserves only 64 bits
+  }
+
+  @Test def testULongFromBigInt: Unit = {
+    val zero = JBigInt.ZERO
+    val one = JBigInt.ONE
+    val two = JBigInt.TWO

Review comment:
       Ah, ok. Pretty easy to just do new JBigInt(2). 

##########
File path: daffodil-lib/src/main/scala/passera/unsigned/ULong.scala
##########
@@ -240,6 +241,19 @@ object ULong {
   val Zero = MinValue
   val MaxValue = ULong(~0L)
 
-  val MaxValueAsBigInt = JBigInt.valueOf(Long.MinValue).abs
-  private val maxULongString = MaxValueAsBigInt.toString()
+  val MaxValueAsBigInt = new JBigInt("FFFFFFFFFFFFFFFF", 16)
+
+  def apply(bi: JBigInt): ULong = {
+    val posBigInt = bi.and(ULong.MaxValueAsBigInt)
+    ULong(posBigInt.longValue)
+  }
+
+  def apply(digits: String, radix: Int): ULong = {
+    if (digits.contains("-") || digits.contains(","))
+      throw new IllegalArgumentException("digits must contain only 0-9A-Fa-F.")

Review comment:
       Good point. We should not check things here, and just let JBigInt do it. 
   This library has that "style" where it tries to be overly accepting of 
arguments and "make them work somehow" rather than my preferred style, which is 
to be as narrow as possible. 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to