Daffodil 1884 bit order #34
Conversation
| final override protected def checkBitOrder(): Unit = { | ||
| ParserBitOrderChecks.checkParseBitOrder(this) |
There was a problem hiding this comment.
All I did here is get rid of the objects ParserBitOrderChecks and UnparserBitOrderChecks and made those methods of PState and UState respectively.
| @@ -116,6 +120,7 @@ trait SuspendableUnparser | |||
| protected def suspendableOperation: SuspendableOperation | |||
|
|
|||
| override final def unparse(state: UState): Unit = { | |||
| state.bitOrder | |||
There was a problem hiding this comment.
I assume this is called so that checkBitOrder is called? Maybe add a comment to that effect?
| this.context match { | ||
| case trd: TermRuntimeData => ustate.dataOutputStream.setPriorBitOrder(ustate.bitOrder) | ||
| case _ => //ok | ||
| } |
There was a problem hiding this comment.
As I recall, we had something similar this.context match { for the parse side of things, but I don't see it now. Maybe I'm misremembering or maybe it was replaced after a code review? Regardless, this doesn't exist on the parse side. Does parsing handle this differently or is this an inherent difference in parse vs. unparse so it is required here?
There was a problem hiding this comment.
They are different. Parsing proceeds in order (ignoring backtracking) so every primitive unparser that could leave one at a frag byte must get its bit order, and it is checked, and checking sets the prior if it is changing.
We did have code at one time like this match-case in unparser in the parser code, but I convinced myself by the above argument that we don't need it.
Unparsing some unparsers are suspendable, so there are a bunch of cases here. Clearly calling this setter here is overkilling the situation. In theory some places in the unparser code dealing with splitting/suspending are missing proper management of the priorBitOrder. Finding those has been problematic. So this is a temporary fix, until I can rationalize the setting of priorBitOrder fully in the unparser.
I will insert comments here accordingly explaining that this is a fix, but inefficient, and should be unnecessary.
No question that a design note is needed here explaining the invariants here, and how it is supposed to work, all of which would be nice to capture when writing the code the first time, but somehow never happens.
I am adding more comments to the code here and in other places explaining what I can about how this works, and hopefully I'll find the issue as a part of that process. If not, well at least all the tests we have are passing.
There was a problem hiding this comment.
Interesting that I can simplify this and just call ustate.bitOrder here. The setPriorBitOrder is a red-herring.
Call ustate.bitOrder after each term unparser. setPriorBitOrder in a few places where it was missing. This corrects an oversight where the unparser was not modified to handle bit order changes properly. Because few data formats change bit orders in the middle of data, this error went unrecognized until it was discovered in NACT Link16 data where the NACT envelope is MSBF, but the Link16 payload is LSBF. Test bitOrderOVC1 exercises changing bit orders where a bit order change occurs, on a byte boundary, but alignment fill is inserted before to fill a partial byte. The alignment fill for the fragment byte must be LSBF even though the element having that alignment fill is MSBF. The tests test_ep1 and test_ep2 are also envelope-payload tests with BE MSBF envelop and LE LSBF payloads. DAFFODIL-1884
mbeckerle
force-pushed
the
daffodil-1884-bitOrder
branch
from
11ce9c2
to
6654c6b
Compare
Though this was approved, I still added to it some comments and a few things that are worth a look.
See commit 3.
There are two commits in this change set.
The first is just code cleanups. No functional change.
The second is the actual changes that fixed the bug DAFFODIL-1884.