Conversation
| 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.
There was a problem hiding this comment.
I'll have to do this in a later PR, or force push so the PRs after this commit doesn't break from the rename
There was a problem hiding this comment.
A force-push shouldn't be necessary for this. You should be able to add a commit to the current branch, and then merge changes from upstream branches to upper PRs.
| } 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.
There was a problem hiding this comment.
IIRC CN depends on this behavior. Will fix in future commit when we decide on removing or keeping error codes. I added this to my note
There was a problem hiding this comment.
I suggest to modify the CN code once the CN is being integrated with the PbjReader. As of now, this current implementation seems to lose an important piece of information about an exception, and I'd prefer we don't lose it.
| } | ||
|
|
||
| @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?
There was a problem hiding this comment.
Oops, this was meant for a later commit. IIRC there was some issue with RandomAccessSequenceAdapter being private but I think I can solve it. I'll remove this
There was a problem hiding this comment.
Please remove it in this PR, so that we don't merge that interface accidentally.
Signed-off-by: ldintr <levo.d@swirldslabs.com>
Signed-off-by: ldintr <levo.d@swirldslabs.com>
2 participants
Stack created with GitHub Stacks CLI • Give Feedback 💬