[GitHub] [commons-imaging] kinow commented on a change in pull request #164: IMAGING-285 RationalNumber to support unsigned int format

[GitBox <gi...@apache.org>]

kinow commented on a change in pull request #164:
URL: https://github.com/apache/commons-imaging/pull/164#discussion_r702279148



##########
File path: src/main/java/org/apache/commons/imaging/common/RationalNumber.java
##########
@@ -73,8 +124,41 @@ private static long gcd(final long a, final long b) {
         return gcd(b, a % b);
     }
 
+    /**
+     * Negates the value of the RationalNumber. If the numerator of this
+     * instance has its high-order bit set, then its value is too large
+     * to be treated as a Java 32-bit signed integer.  In such a case, the
+     * only way that a RationalNumber instance can be negated is to
+     * sacrifice one bit of precision by dividing both the numerator and
+     * the denominator by 2. However, if that computation would result
+     * in a zero denominator, then the value cannot be negated and an
+     * exception is thrown.
+     * @return a valid instance with a negated value.
+     */
     public RationalNumber negate() {
-        return new RationalNumber(-numerator, divisor);
+        long n = numerator;
+        long d = divisor;
+        if (unsignedType) {
+            // An instance of an unsigned type can be negated if and only if
+            // its high-order bit (the sign bit) is clear. If the bit is set,
+            // it is necessary to adjust the

Review comment:
       to adjust the?

##########
File path: src/main/java/org/apache/commons/imaging/common/RationalNumber.java
##########
@@ -73,8 +124,41 @@ private static long gcd(final long a, final long b) {
         return gcd(b, a % b);
     }
 
+    /**
+     * Negates the value of the RationalNumber. If the numerator of this
+     * instance has its high-order bit set, then its value is too large
+     * to be treated as a Java 32-bit signed integer.  In such a case, the
+     * only way that a RationalNumber instance can be negated is to
+     * sacrifice one bit of precision by dividing both the numerator and
+     * the denominator by 2. However, if that computation would result

Review comment:
       by `2` or by the greatest common divisor?

##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/fieldtypes/FieldTypeRational.java
##########
@@ -31,11 +31,19 @@ public FieldTypeRational(final int type, final String name) {
     @Override
     public Object getValue(final TiffField entry) {
         final byte[] bytes = entry.getByteArrayValue();
+        boolean unsignedType = true;
+        if(entry.getFieldType()==RATIONAL){
+            unsignedType = true;
+        }else if(entry.getFieldType()==SRATIONAL){
+            unsignedType = false;
+        }

Review comment:
       What about:
   
   ```suggestion
           boolean unsignedType = entry.getFieldType() != SRATIONAL;
   ```

##########
File path: src/main/java/org/apache/commons/imaging/common/RationalNumber.java
##########
@@ -20,6 +20,14 @@
 
 /**
  * Rational number, as used by the TIFF image format.
+ * <p>
+ * The TIFF format specifies two data types for rational numbers based on
+ * a pair of 32-bit integers.  Rational is based on unsigned 32-bit integers
+ * and SRational is based on signed 32-bit integers.  This treatment is
+ * problematic in Java because Java does not support unsigned types.
+ * To address this challenge, this class stores the numerator and divisor
+ * in long (64-bit) integers, applying masks as necessary for the unsigned
+ * type.

Review comment:
       :+1: thanks for the useful comment!




-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org