Conversation
Signed-off-by: ldintr <levo.d@swirldslabs.com>
| public void setPbjReader(@NonNull PbjReader reader) { | ||
| reader.resetWith(buffer, start, start + length); |
There was a problem hiding this comment.
set may not be the perfect verb for this method because it's not a setter, i.e. it doesn't update the internal state of the Bytes object. resetPbjReader might sound better.
Also, you want to add a javadoc for this new public method.
| private static final boolean useStacktrace = | ||
| !"false".equalsIgnoreCase(System.getProperty("pbj.ReaderWriter.useStackTrace")); | ||
|
|
||
| public static final int EOF = -1, | ||
| Closed = -2, | ||
| DataEncoding = 1, | ||
| BufferUnderflow = 2, | ||
| Parse = 3, | ||
| IllegalArgument = 4, | ||
| IOError = 5, | ||
| Unsupported = 6, // used with WIRE_TYPE_GROUP_START, WIRE_TYPE_GROUP_END | ||
| UsageError = 9, | ||
| UnknownField = 10, | ||
| BufferOverflow = 11, | ||
| MaxDepthReached = 12, | ||
| MalformString = 13; | ||
|
|
||
| private static final UnknownFieldException premadeUnknown; | ||
| private static final BufferUnderflowException premadeUnderflow; | ||
| private static final BufferOverflowException premadeOverflow; | ||
| private static final RuntimeException premadeRuntime, premadeUnsupported; | ||
| private static final DataEncodingException premadeDataEncoding; | ||
| private static final IllegalArgumentException premadeIllegal; | ||
| private static final ParseException premadeParseEmpty, premadeParseUnknown, premadeMaxDepth; |
There was a problem hiding this comment.
static members in Java should all be UPPER_SNAKE_CASE. Otherwise, it's too easy to confuse them with instance members or local variables.
| public long readVarLongNoZZ() { | ||
| if (pos + 10 <= relLimit) { | ||
| long value = 0; | ||
| for (int i = 0; i < 10; i++) { |
There was a problem hiding this comment.
For reading varints, we want to use the algorithm that we've determined is the fastest and which is implemented elsewhere in PBJ.
| } else if (errorKind == Parse) { | ||
| cause = new RuntimeException(message); | ||
| includeCause = false; |
There was a problem hiding this comment.
I'm still concerned about setting includeCause to false in this branch. To me it seems as though we're going to lose some context-related information.
| } | ||
|
|
||
| @Override | ||
| public void close() { |
There was a problem hiding this comment.
I've already seen this code a couple of times in previous iterations, so I mostly just skimmed it through this time. However, I'd like @artemananiev to review it thoroughly this time.
| /** | ||
| * Implemented by {@link ReadableSequentialData} types that are backed directly by a heap byte array. | ||
| * Callers (e.g. {@link com.hedera.pbj.runtime.io.buffer.PbjReader}) can detect this and use the array directly | ||
| * instead of copying data through an intermediate buffer. | ||
| */ | ||
| public interface ByteArraySequentialData { | ||
| /** The raw backing byte array. */ | ||
| byte[] byteArrayUnsafe(); |
There was a problem hiding this comment.
As discussed previously, these operations look unsafe and we'd prefer not to introduce such APIs. Can we not use an approach similar to what's done with Bytes in this PR?
2 participants
Stack created with GitHub Stacks CLI • Give Feedback 💬