Adds support for parsing binary argument groups. #910
Conversation
| ArgumentGroupMarker[] argumentGroupStack = new ArgumentGroupMarker[CONTAINER_STACK_INITIAL_CAPACITY]; | ||
| int argumentGroupIndex = -1; |
There was a problem hiding this comment.
Just curious—we have a _Private_RecyclingStack that looks to do about the same thing as what you are doing with this. Have you observed any performance difference between the two?
There was a problem hiding this comment.
Yes, unfortunately. I originally used the recycling stack for the containers in IonCursorBinary and observed a fairly significant speedup (~10% as I recall) by using an array directly. I decided to use the same technique here.
| // Push a dummy delimited container onto the stack, preparing the cursor to seek forward to the delimited end | ||
| // marker applicable at the current depth. | ||
| pushContainer(); |
There was a problem hiding this comment.
What is your reasoning for not actually treating expression groups like a container?
If you just treated expression groups like another container, it seems like you could probably eliminate some data structures, some code that is similar between containers and expression groups, and some public methods.
There was a problem hiding this comment.
I considered implementing it that way, but decided not to (for now) for a few reasons:
- It complicates calculating of the reader's current depth because the argument groups need to be filtered out. This is pretty minor.
- The container stack uses the Marker class, which is reused in many places and has a startIndex, endIndex, and IonTypeID. Argument groups need a startIndex, endIndex, and PrimitiveType. I'd have to merge these classes if using a unified stack, and I didn't feel like bloating Marker with an attribute that is only needed by argument groups (and is never needed in Ion 1.0). This also isn't a huge deal.
Which public methods were you envisioning this would allow us to remove? I'm not sure we could reuse the stepIn/stepOut methods for this, for example, because the encoding for tagged and tagless groups differs and is determined by the signature.
Anyway, I'm happy to revisit this in the future, but if you're up for merging this as-is for now we can start using it and see whether it ends up being cumbersome.
Co-authored-by: Matthew Pope <[email protected]>
Description of changes:
Implements the low-level parsing of binary argument groups according to the following spec:
The public methods added to IonCursorBinary are expected to be called by higher-level readers with awareness of the macro signatures applicable at the current stream position. The basic sequence of method calls required to parse tagged and tagless argument groups is illustrated in the added tests.
At the bottom of the tests I've added some TODOs to provide more coverage of edge cases. More thorough coverage will also come from the conformance suite. In the source I also added some TODOs regarding the challenges of skipping values that may contain non-prefixed macro invocations. We'll need to handle this somehow (via consultation with a higher-level reader with knowledge of the signatures), but I've left that out of scope for this PR.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.