DonalEvans commented on a change in pull request #6765:
URL: https://github.com/apache/geode/pull/6765#discussion_r690750209
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
##########
@@ -471,4 +472,101 @@ public static int narrowLongToInt(long toBeNarrowed) {
}
return doubleBytes;
}
+
+ /**
+ * Takes the given "value" and computes the sequence of ASCII digits it
represents,
+ * appending them to the given "buf".
+ * This code was adapted from the openjdk Long.java getChars methods.
+ */
+ public static void appendAsciiDigitsToByteBuf(long value, ByteBuf buf) {
+ if (value < 0) {
+ buf.writeByte('-');
Review comment:
This char can be replaced with the `bMINUS` constant.
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
##########
@@ -471,4 +472,101 @@ public static int narrowLongToInt(long toBeNarrowed) {
}
return doubleBytes;
}
+
+ /**
+ * Takes the given "value" and computes the sequence of ASCII digits it
represents,
+ * appending them to the given "buf".
+ * This code was adapted from the openjdk Long.java getChars methods.
+ */
+ public static void appendAsciiDigitsToByteBuf(long value, ByteBuf buf) {
+ if (value < 0) {
+ buf.writeByte('-');
+ } else {
+ value = -value;
+ }
+ // at this point value <= 0
+
+ if (value > -100) {
+ // it has at most two digits: [0..99]
+ appendSmallAsciiDigitsToByteBuf((int) value, buf);
+ } else {
+ appendLargeAsciiDigitsToByteBuf(value, buf);
+ }
+ }
+
+ private static void appendSmallAsciiDigitsToByteBuf(int value, ByteBuf buf) {
+ int q = value / 10;
+ int r = (q * 10) - value;
+ if (q < 0) {
+ buf.writeByte((byte) ('0' - q));
+ }
+ buf.writeByte((byte) ('0' + r));
Review comment:
These zeroes can be replaced with the `NUMBER_0_BYTE` constant.
##########
File path:
geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/netty/CoderTest.java
##########
@@ -176,4 +180,25 @@ public void
stripTrailingZeroFromDouble_correctlyStripsTrailingZero(byte[] input
};
}
+ @Test
+ public void verify_appendAsciiDigitsToByteBuf_conversions() {
+ verify_appendAsciiDigitsToByteBuf(Long.MAX_VALUE);
+ verify_appendAsciiDigitsToByteBuf(Long.MIN_VALUE);
+ verify_appendAsciiDigitsToByteBuf(Integer.MAX_VALUE);
+ verify_appendAsciiDigitsToByteBuf(Integer.MIN_VALUE);
+ verify_appendAsciiDigitsToByteBuf(Short.MAX_VALUE);
+ verify_appendAsciiDigitsToByteBuf(Short.MIN_VALUE);
+ for (long i = Byte.MIN_VALUE; i <= Byte.MAX_VALUE; i++) {
+ verify_appendAsciiDigitsToByteBuf(i);
+ }
+ }
+
+ private void verify_appendAsciiDigitsToByteBuf(long value) {
+ String expected = Long.toString(value);
+ ByteBuf buf = ByteBufAllocator.DEFAULT.heapBuffer();
+
+ Coder.appendAsciiDigitsToByteBuf(value, buf);
+
+ assertThat(buf.toString(Charset.defaultCharset())).isEqualTo(expected);
Review comment:
I think we might want to explicitly use UTF-8 as the charset here, as
that's what Redis and the Coder class use.
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
##########
@@ -471,4 +472,101 @@ public static int narrowLongToInt(long toBeNarrowed) {
}
return doubleBytes;
}
+
+ /**
+ * Takes the given "value" and computes the sequence of ASCII digits it
represents,
+ * appending them to the given "buf".
+ * This code was adapted from the openjdk Long.java getChars methods.
+ */
+ public static void appendAsciiDigitsToByteBuf(long value, ByteBuf buf) {
+ if (value < 0) {
+ buf.writeByte('-');
+ } else {
+ value = -value;
+ }
+ // at this point value <= 0
+
+ if (value > -100) {
+ // it has at most two digits: [0..99]
+ appendSmallAsciiDigitsToByteBuf((int) value, buf);
+ } else {
+ appendLargeAsciiDigitsToByteBuf(value, buf);
+ }
+ }
+
+ private static void appendSmallAsciiDigitsToByteBuf(int value, ByteBuf buf) {
+ int q = value / 10;
+ int r = (q * 10) - value;
+ if (q < 0) {
+ buf.writeByte((byte) ('0' - q));
+ }
+ buf.writeByte((byte) ('0' + r));
+ }
+
+ private static void appendLargeAsciiDigitsToByteBuf(long value, ByteBuf buf)
{
+ long q;
+ int r;
+ final int MAX_DIGITS = 19;
+ int charPos = MAX_DIGITS;
+ byte[] bytes = new byte[MAX_DIGITS];
+
+ // Get 2 digits/iteration using longs until quotient fits into an int
+ while (value <= Integer.MIN_VALUE) {
+ q = value / 100;
+ r = (int) ((q * 100) - value);
+ value = q;
+ bytes[--charPos] = DIGIT_ONES[r];
+ bytes[--charPos] = DIGIT_TENS[r];
+ }
+
+ // Get 2 digits/iteration using ints
+ int q2;
+ int i2 = (int) value;
+ while (i2 <= -100) {
+ q2 = i2 / 100;
+ r = (q2 * 100) - i2;
+ i2 = q2;
+ bytes[--charPos] = DIGIT_ONES[r];
+ bytes[--charPos] = DIGIT_TENS[r];
+ }
+
+ // We know there are at most two digits left at this point.
+ q2 = i2 / 10;
+ r = (q2 * 10) - i2;
+ bytes[--charPos] = (byte) ('0' + r);
+
+ // Whatever left is the remaining digit.
+ if (q2 < 0) {
+ bytes[--charPos] = (byte) ('0' - q2);
+ }
+ buf.writeBytes(bytes, charPos, MAX_DIGITS - charPos);
+ }
+
+ @Immutable
+ private static final byte[] DIGIT_TENS = {
Review comment:
If we want to follow the "important note" about not using chars
mentioned above, then these arrays will need to use the UTF-8 byte values
associated with these chars, which will probably make it look horrible.
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
##########
@@ -471,4 +472,101 @@ public static int narrowLongToInt(long toBeNarrowed) {
}
return doubleBytes;
}
+
+ /**
+ * Takes the given "value" and computes the sequence of ASCII digits it
represents,
+ * appending them to the given "buf".
+ * This code was adapted from the openjdk Long.java getChars methods.
+ */
+ public static void appendAsciiDigitsToByteBuf(long value, ByteBuf buf) {
+ if (value < 0) {
+ buf.writeByte('-');
+ } else {
+ value = -value;
+ }
+ // at this point value <= 0
+
+ if (value > -100) {
+ // it has at most two digits: [0..99]
+ appendSmallAsciiDigitsToByteBuf((int) value, buf);
+ } else {
+ appendLargeAsciiDigitsToByteBuf(value, buf);
+ }
+ }
+
+ private static void appendSmallAsciiDigitsToByteBuf(int value, ByteBuf buf) {
+ int q = value / 10;
+ int r = (q * 10) - value;
+ if (q < 0) {
+ buf.writeByte((byte) ('0' - q));
+ }
+ buf.writeByte((byte) ('0' + r));
+ }
+
+ private static void appendLargeAsciiDigitsToByteBuf(long value, ByteBuf buf)
{
+ long q;
+ int r;
+ final int MAX_DIGITS = 19;
+ int charPos = MAX_DIGITS;
+ byte[] bytes = new byte[MAX_DIGITS];
+
+ // Get 2 digits/iteration using longs until quotient fits into an int
+ while (value <= Integer.MIN_VALUE) {
+ q = value / 100;
+ r = (int) ((q * 100) - value);
+ value = q;
+ bytes[--charPos] = DIGIT_ONES[r];
+ bytes[--charPos] = DIGIT_TENS[r];
+ }
+
+ // Get 2 digits/iteration using ints
+ int q2;
+ int i2 = (int) value;
+ while (i2 <= -100) {
+ q2 = i2 / 100;
+ r = (q2 * 100) - i2;
+ i2 = q2;
+ bytes[--charPos] = DIGIT_ONES[r];
+ bytes[--charPos] = DIGIT_TENS[r];
+ }
+
+ // We know there are at most two digits left at this point.
+ q2 = i2 / 10;
+ r = (q2 * 10) - i2;
+ bytes[--charPos] = (byte) ('0' + r);
+
+ // Whatever left is the remaining digit.
+ if (q2 < 0) {
+ bytes[--charPos] = (byte) ('0' - q2);
Review comment:
These zeroes can be replaced with the `NUMBER_0_BYTE` constant.
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
##########
@@ -155,12 +156,12 @@ private static void writeCollectionOrString(ByteBuf
buffer, Object next) throws
public static ByteBuf getScanResponse(ByteBuf buffer, BigInteger cursor,
List<?> scanResult) {
buffer.writeByte(ARRAY_ID);
- buffer.writeBytes(intToBytes(2));
+ buffer.writeByte('2');
Review comment:
Per a comment at the top of `StringBytesGlossary` (which was originally
in this class before being pulled out):
```
* Important note
* <p>
* Do not use '' <-- java primitive chars. Redis uses {@link Coder#CHARSET}
encoding so we should
* not risk java handling char to byte conversions, rather just hard code
{@link Coder#CHARSET}
* chars as bytes
```
Do we still want to follow this? If so, I think the solution is to introduce
a `byte` constant to the `StringBytesGlossary` class for this char. We already
have constants for 0 and 1, so this wouldn't be too out of the ordinary.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
