Skip to content

Use an empty sequence for the xml of implied choice branch sequences#1219

Merged
stevedlawrence merged 1 commit intoapache:mainfrom
stevedlawrence:daffodil-2887-expression-out-of-choice
Jun 4, 2024
Merged

Use an empty sequence for the xml of implied choice branch sequences#1219
stevedlawrence merged 1 commit intoapache:mainfrom
stevedlawrence:daffodil-2887-expression-out-of-choice

Conversation

@stevedlawrence
Copy link
Member

When we need to add an implied sequence for a choice branch (like when the branch is an array), we wrap the branch Term in a ChoiceBranchImpliedSequence, which is essentially a Sequence that passes logic through to the underlying Term. This allows us to use existing sequence logic such as creating repetition parsers for arrays.

However, when we create this ChoiceBranchImpliedSequence we set its xml parameter to the xml of the Term it wraps. And because it extends Sequence, it does all the normal sequence logic of parsing that xml, even though much of it is never used because most logic passes through to the underlying Term.

In most cases, this works fine (although its wasted effort) since all that extra work is simply ignored. But, if that XML contains any expressions, things can quickly go off the rails as it tries to compile expressions with the context of this implied sequence.

To fix this, instead of passing in the XML of the underlying Term, we just pass in an empty sequence XML. This means there is no longer any real work for the ChoiceBranchImpliedSequence to do, except for be a tool to create the necessary repetition parsers to wrap around the Term. Not only does this avoid issues with incorrect contexts, but it avoids unnecessary work that is just ignored.

DAFFODIL-2887

When we need to add an implied sequence for a choice branch (like when
the branch is an array), we wrap the branch Term in a
ChoiceBranchImpliedSequence, which is essentially a Sequence that passes
logic through to the underlying Term. This allows us to use existing
sequence logic such as creating repetition parsers for arrays.

However, when we create this ChoiceBranchImpliedSequence we set its xml
parameter to the xml of the Term it wraps. And because it extends
Sequence, it does all the normal sequence logic of parsing that xml,
even though much of it is never used because most logic passes through
to the underlying Term.

In most cases, this works fine (although its wasted effort) since all
that extra work is simply ignored. But, if that XML contains any
expressions, things can quickly go off the rails as it tries to compile
expressions with the context of this implied sequence.

To fix this, instead of passing in the XML of the underlying Term, we
just pass in an empty sequence XML. This means there is no longer any
real work for the ChoiceBranchImpliedSequence to do, except for be a
tool to create the necessary repetition parsers to wrap around the Term.
Not only does this avoid issues with incorrect contexts, but it avoids
unnecessary work that is just ignored.

DAFFODIL-2887
Copy link
Contributor

@tuxji tuxji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Contributor

@mbeckerle mbeckerle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@stevedlawrence stevedlawrence merged commit 2037fe9 into apache:main Jun 4, 2024
@stevedlawrence stevedlawrence deleted the daffodil-2887-expression-out-of-choice branch June 4, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants