You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2022/04/07 08:50:13 UTC
[GitHub] [commons-lang] arturobernalg opened a new pull request, #877: Simplify and remove unnecessary check.
arturobernalg opened a new pull request, #877:
URL: https://github.com/apache/commons-lang/pull/877
Simplify and remove unnecessary check that always is true
`src.length > srcPos`
--
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
[GitHub] [commons-lang] arturobernalg commented on a diff in pull request #877: Simplify and remove unnecessary check.
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on code in PR #877:
URL: https://github.com/apache/commons-lang/pull/877#discussion_r883071033
##########
src/test/java/org/apache/commons/lang3/ConversionTest.java:
##########
@@ -413,6 +415,29 @@ public void testBinaryBeMsb0ToHexDigit_2args() {
}
+ @Test
+ public void testBinaryToHexDigitReverse() {
+ final SplittableRandom rng = new SplittableRandom();
+ final boolean[] x = new boolean[8];
+ for (int i = 0; i < 100; i++) {
+ Conversion.longToBinary(rng.nextLong(), 0, x, 0, 8);
+ for (int j = 1; j <= 8; j++) {
+ final boolean[] a = Arrays.copyOf(x, j);
+ final boolean[] b = a.clone();
+ ArrayUtils.reverse(b);
+ for (int k = 0; k < j; k++) {
+ assertEquals(Conversion.binaryToHexDigit(a, k),
+ Conversion.binaryBeMsb0ToHexDigit(b, k));
+ }
+ }
+ }
+ }
+
+ @Test
+ public void binaryBeMsb0ToHexDigitPosOutsideArray() {
+ assertThrows(IndexOutOfBoundsException.class, () -> Conversion.binaryBeMsb0ToHexDigit(new boolean[8], 99));
Review Comment:
done
--
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
[GitHub] [commons-lang] aherbert commented on pull request #877: Simplify and remove unnecessary check.
Posted by GitBox <gi...@apache.org>.
aherbert commented on PR #877:
URL: https://github.com/apache/commons-lang/pull/877#issuecomment-1139717900
Closed by cf7c9522aa99d0d180e54ac5abe849f19d153d25
--
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
[GitHub] [commons-lang] arturobernalg commented on a diff in pull request #877: Simplify and remove unnecessary check.
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on code in PR #877:
URL: https://github.com/apache/commons-lang/pull/877#discussion_r845453817
##########
src/main/java/org/apache/commons/lang3/Conversion.java:
##########
@@ -447,16 +447,16 @@ public static char binaryBeMsb0ToHexDigit(boolean[] src, int srcPos) {
src = paddedSrc;
srcPos = 0;
Review Comment:
HI @aherbert
That is actually a great approach. Did't see that.
Changed.
TY
--
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
[GitHub] [commons-lang] arturobernalg commented on pull request #877: Simplify and remove unnecessary check.
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on PR #877:
URL: https://github.com/apache/commons-lang/pull/877#issuecomment-1139319443
> Can you rebase this on master please.
done
--
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
[GitHub] [commons-lang] aherbert closed pull request #877: Simplify and remove unnecessary check.
Posted by GitBox <gi...@apache.org>.
aherbert closed pull request #877: Simplify and remove unnecessary check.
URL: https://github.com/apache/commons-lang/pull/877
--
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
[GitHub] [commons-lang] aherbert commented on a diff in pull request #877: Simplify and remove unnecessary check.
Posted by GitBox <gi...@apache.org>.
aherbert commented on code in PR #877:
URL: https://github.com/apache/commons-lang/pull/877#discussion_r883054287
##########
src/test/java/org/apache/commons/lang3/ConversionTest.java:
##########
@@ -413,6 +415,29 @@ public void testBinaryBeMsb0ToHexDigit_2args() {
}
+ @Test
+ public void testBinaryToHexDigitReverse() {
+ final SplittableRandom rng = new SplittableRandom();
+ final boolean[] x = new boolean[8];
+ for (int i = 0; i < 100; i++) {
+ Conversion.longToBinary(rng.nextLong(), 0, x, 0, 8);
+ for (int j = 1; j <= 8; j++) {
+ final boolean[] a = Arrays.copyOf(x, j);
+ final boolean[] b = a.clone();
+ ArrayUtils.reverse(b);
+ for (int k = 0; k < j; k++) {
+ assertEquals(Conversion.binaryToHexDigit(a, k),
+ Conversion.binaryBeMsb0ToHexDigit(b, k));
+ }
+ }
+ }
+ }
+
+ @Test
+ public void binaryBeMsb0ToHexDigitPosOutsideArray() {
+ assertThrows(IndexOutOfBoundsException.class, () -> Conversion.binaryBeMsb0ToHexDigit(new boolean[8], 99));
Review Comment:
I would have just added to the existing test:
```Java
@Test
public void binaryBeMsb0ToHexDigitPosOutsideArray() {
final boolean[] array = new boolean[8];
for (final int index : new int[] {-1, array.length, array.length + 100}) {
assertThrows(IndexOutOfBoundsException.class,
() -> Conversion.binaryBeMsb0ToHexDigit(array, index),
() -> "Index: " + index;
);
}
}
```
Or perhaps use a parameterized test:
```Java
@ParameterizedTest
@ValueSource(ints = {-1, 8, 99})
public void binaryBeMsb0ToHexDigitPosOutsideArray(int index) {
assertThrows(IndexOutOfBoundsException.class,
() -> Conversion.binaryBeMsb0ToHexDigit(new boolean[8], index));
}
```
Anyway can you rebase this on master and then it is good to merge. Thanks.
--
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
[GitHub] [commons-lang] arturobernalg commented on a diff in pull request #877: Simplify and remove unnecessary check.
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on code in PR #877:
URL: https://github.com/apache/commons-lang/pull/877#discussion_r882849172
##########
src/main/java/org/apache/commons/lang3/Conversion.java:
##########
@@ -435,39 +435,40 @@ public static char binaryBeMsb0ToHexDigit(final boolean[] src) {
* @return a hexadecimal digit representing the selected bits
* @throws IllegalArgumentException if {@code src} is empty
* @throws NullPointerException if {@code src} is {@code null}
+ * @throws IndexOutOfBoundsException if {@code srcPos} is outside the array.
*/
public static char binaryBeMsb0ToHexDigit(boolean[] src, int srcPos) {
if (src.length == 0) {
Review Comment:
done
##########
src/test/java/org/apache/commons/lang3/ConversionTest.java:
##########
@@ -413,6 +415,29 @@ public void testBinaryBeMsb0ToHexDigit_2args() {
}
+ @Test
+ public void testBinaryToHexDigitReverse() {
+ final SplittableRandom rng = new SplittableRandom();
+ final boolean[] x = new boolean[8];
+ for (int i = 0; i < 100; i++) {
+ Conversion.longToBinary(rng.nextLong(), 0, x, 0, 8);
+ for (int j = 1; j <= 8; j++) {
+ final boolean[] a = Arrays.copyOf(x, j);
+ final boolean[] b = a.clone();
+ ArrayUtils.reverse(b);
+ for (int k = 0; k < j; k++) {
+ assertEquals(Conversion.binaryToHexDigit(a, k),
+ Conversion.binaryBeMsb0ToHexDigit(b, k));
+ }
+ }
+ }
+ }
+
+ @Test
+ public void binaryBeMsb0ToHexDigitPosOutsideArray() {
+ assertThrows(IndexOutOfBoundsException.class, () -> Conversion.binaryBeMsb0ToHexDigit(new boolean[8], 99));
Review Comment:
done
--
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
[GitHub] [commons-lang] aherbert commented on a diff in pull request #877: Simplify and remove unnecessary check.
Posted by GitBox <gi...@apache.org>.
aherbert commented on code in PR #877:
URL: https://github.com/apache/commons-lang/pull/877#discussion_r873216278
##########
src/test/java/org/apache/commons/lang3/ConversionTest.java:
##########
@@ -413,6 +415,29 @@ public void testBinaryBeMsb0ToHexDigit_2args() {
}
+ @Test
+ public void testBinaryToHexDigitReverse() {
+ final SplittableRandom rng = new SplittableRandom();
+ final boolean[] x = new boolean[8];
+ for (int i = 0; i < 100; i++) {
+ Conversion.longToBinary(rng.nextLong(), 0, x, 0, 8);
+ for (int j = 1; j <= 8; j++) {
+ final boolean[] a = Arrays.copyOf(x, j);
+ final boolean[] b = a.clone();
+ ArrayUtils.reverse(b);
+ for (int k = 0; k < j; k++) {
+ assertEquals(Conversion.binaryToHexDigit(a, k),
+ Conversion.binaryBeMsb0ToHexDigit(b, k));
+ }
+ }
+ }
+ }
+
+ @Test
+ public void binaryBeMsb0ToHexDigitPosOutsideArray() {
+ assertThrows(IndexOutOfBoundsException.class, () -> Conversion.binaryBeMsb0ToHexDigit(new boolean[8], 99));
Review Comment:
Add a test for the index as negative or a match to the array length.
##########
src/main/java/org/apache/commons/lang3/Conversion.java:
##########
@@ -435,39 +435,40 @@ public static char binaryBeMsb0ToHexDigit(final boolean[] src) {
* @return a hexadecimal digit representing the selected bits
* @throws IllegalArgumentException if {@code src} is empty
* @throws NullPointerException if {@code src} is {@code null}
+ * @throws IndexOutOfBoundsException if {@code srcPos} is outside the array.
*/
public static char binaryBeMsb0ToHexDigit(boolean[] src, int srcPos) {
if (src.length == 0) {
Review Comment:
To leave only one branch check on the main path I would put this check inside the other check:
```Java
if (Integer.compareUnsigned(srcPos, src.length) >= 0) {
// Throw the correct exception
if (src.length == 0) {
throw new IllegalArgumentException("Cannot convert an empty array.");
}
throw new IndexOutOfBoundsException(srcPos + " is not within array length " + src.length);
}
```
--
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
[GitHub] [commons-lang] aherbert commented on a diff in pull request #877: Simplify and remove unnecessary check.
Posted by GitBox <gi...@apache.org>.
aherbert commented on code in PR #877:
URL: https://github.com/apache/commons-lang/pull/877#discussion_r845025971
##########
src/main/java/org/apache/commons/lang3/Conversion.java:
##########
@@ -447,16 +447,16 @@ public static char binaryBeMsb0ToHexDigit(boolean[] src, int srcPos) {
src = paddedSrc;
srcPos = 0;
Review Comment:
This method appears to have been rewritten from a version that used `src` directly to a version that pads `src` to 4 values. However after the rewrite all the redundant length checks have not been removed. Looking at the git history the original version also had the padding. So this is a strange legacy bit of code from 10 years ago.
IIUC the `srcPos` is set to zero and then never changed after this point. So its use could be removed, e.g. `srcPos + 3` is the same as `3`. Likewise all the checks against length are not required as `paddedSrc` is length 4. So you have removed some of them but not the rest starting at line 461.
However a further look into this finds that this method and `binaryToHexDigit` are essentially the same method but with the endianness inverted. One method starts at the given position for the the lsb and increments the position up to 3 times to read the boolean bits. The other starts at the provided lsb counting **from the end** of the input array (this is not explicitly documented but is demonstrated in the javadoc) and decrements up to 3 times. The later method does this by extracting the range into a padded array of 4, the full logic of which has some issues when the provided srcPos is not inside the array (this is not tested in the unit tests).
I think it makes more sense to remove the padding to 4 characters and leave the length checks in place. The method should be the same as `binaryToHexDigit` but with the indexing reversed. Thus:
```Java
boolean[] src = ...;
int srcPos = ...;
boolean[] reversed = src.clone();
ArrayUtils.reverse(reversed);
binaryToHexDigit(src, srcPos) == binaryBeMsb0ToHexDigit(reversed, srcPos);
```
I've tested the above and it seems to be the intention of the the method:
```Java
@Test
public void testBinaryToHexDigitReverse() {
SplittableRandom rng = new SplittableRandom();
boolean[] x = new boolean[8];
for (int i = 0; i < 100; i++) {
Conversion.longToBinary(rng.nextLong(), 0, x, 0, 8);
for (int j = 1; j <= 8; j++) {
boolean[] a = Arrays.copyOf(x, j);
boolean[] b = a.clone();
ArrayUtils.reverse(b);
for (int k = 0; k < j; k++) {
Assertions.assertEquals(Conversion.binaryToHexDigit(a, k),
Conversion.binaryBeMsb0ToHexDigit(b, k));
}
}
}
}
```
The unit tests for these methods do not cover using a bad input `srcPos` outside the array. In the case of `binaryToHexDigit` it would throw an IOOBE when using `src[srcPos]`. In the case of `binaryBeMsb0ToHexDigit` the padding logic may throw an exception but I have not checked.
I would update both methods with a check that `srcPos` is inside the input array, something like the `Objects.checkIndex` method from JDK 9:
```Java
if (Integer.compareUnsigned(srcPos, src.length) >= 0) {
throw new IndexOutOfBoundsException(srcPos + " is not within array length " + src.length);
}
```
This would make the check on `src.length` redundant as 0 is not within an array of length 0.
The updated method becomes:
```Java
public static char binaryBeMsb0ToHexDigit(boolean[] src, int srcPos) {
if (src.length == 0) {
throw new IllegalArgumentException("Cannot convert an empty array.");
}
// JDK 9: Objects.checkIndex(int index, int length)
if (Integer.compareUnsigned(srcPos, src.length) >= 0) {
throw new IndexOutOfBoundsException(srcPos + " is not within array length " + src.length);
}
// Little-endian bit 0 position
final int pos = src.length - 1 - srcPos;
if (3 <= pos && src[pos - 3]) {
if (src[pos - 2]) {
if (src[pos - 1]) {
return src[pos] ? 'f' : 'e';
}
return src[pos] ? 'd' : 'c';
}
if (src[pos - 1]) {
return src[pos] ? 'b' : 'a';
}
return src[pos] ? '9' : '8';
}
if (2 <= pos && src[pos - 2]) {
if (src[pos - 1]) {
return src[pos] ? '7' : '6';
}
return src[pos] ? '5' : '4';
}
if (1 <= pos && src[pos - 1]) {
return src[pos] ? '3' : '2';
}
return src[pos] ? '1' : '0';
```
This passes the existing unit tests and the extra test for bit reversal shown above. The check for a zero length array is redundant with the index test but I left it in as it would be a breaking change. The existing unit tests do expect an IllegalArgumentException for this case.
It has the advantage that it is clearly the same as `binaryToHexDigit` but working from the end of the array backwards, i.e. the input array is big-endian most-significant-byte at position 0 (BeMsb0)
--
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
[GitHub] [commons-lang] aherbert commented on pull request #877: Simplify and remove unnecessary check.
Posted by GitBox <gi...@apache.org>.
aherbert commented on PR #877:
URL: https://github.com/apache/commons-lang/pull/877#issuecomment-1139106540
Can you rebase this on master please.
--
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