kennknowles commented on code in PR #34542:
URL: https://github.com/apache/beam/pull/34542#discussion_r2029160715
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/util/VarInt.java:
##########
@@ -102,7 +102,7 @@ public static void encode(long v, OutputStream stream)
throws IOException {
/** Decodes an integer value from the given stream. */
public static int decodeInt(InputStream stream) throws IOException {
long r = decodeLong(stream);
- if (r < 0 || r >= 1L << 32) {
Review Comment:
OK my opinions are more like:
1. Use more parentheses when mixing `<` and `>=` and `<<` 😅 .
2. If the bytes on the stream are a valid `VarInt` in the right range then
they should decode correctly, otherwise they shouldn't.
3. I think re-using the logic of `decodeLong` makes things less clear. We
have this problem but also the error message is less specific (always says
"varint"). It would be better to duplicate the logic so `decodeInt` and
`decodeLong` are independent.
4. This loop has so few iterations that it could be unrolled and it would be
perfectly clear and potentially faster.
Perhaps these are not the answers to the question you really are asking, but
that is my opinion of how to improve the code here.
--
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: github-unsubscr...@beam.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
