You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by "jadams-tresys (via GitHub)" <gi...@apache.org> on 2023/02/21 17:58:51 UTC
[GitHub] [daffodil] jadams-tresys opened a new pull request, #967: Consider ChoiceGroupRef's open when determining next elements
jadams-tresys opened a new pull request, #967:
URL: https://github.com/apache/daffodil/pull/967
When determining possible following elements for a group that is a choice, it is important to consider them Open as the group can be shared and have multiple contexts. The way things had worked before is we would push TermRuntimeData's for each group reference onto the stack when looking for the next possible element. However, with since these groups are actually choices with required elements in every branch they were considered closed and if there was an issue attempting to find a following element with the first group ref's TRD we would create an InvalidErrorERD instead of continuing down the stack until we got to the next group ref.
DAFFODIL-2615
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] jadams-tresys merged pull request #967: Consider ChoiceGroupRef's open when determining next elements
Posted by "jadams-tresys (via GitHub)" <gi...@apache.org>.
jadams-tresys merged PR #967:
URL: https://github.com/apache/daffodil/pull/967
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on a diff in pull request #967: Consider ChoiceGroupRef's open when determining next elements
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #967:
URL: https://github.com/apache/daffodil/pull/967#discussion_r1116251273
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/TermRuntime1Mixin.scala:
##########
@@ -354,8 +355,12 @@ trait TermRuntime1Mixin { self: Term =>
case (Closed(pnes1), Closed(pnes2)) => {
thisItself // When everything is closed, we only want thisItself and not the entire list of Closed possibilities
}
- case (Open(pnes1), Closed(pnes2)) => {
+ case (Open(pnes1), Closed(pnes2)) if !this.isInstanceOf[ChoiceGroupRef] => {
// If there are optional elements before a required element, the whole sequence is considered closed
+ //
+ // If this is a ChoiceGroupRef though, it could be one of multiple group references and thus we may
+ // not have the appropriate context to find the expected following element, so we should remain Open
Review Comment:
This comment says "so we should remain open", but the next line of code is a Closed.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/TermRuntime1Mixin.scala:
##########
@@ -354,8 +355,12 @@ trait TermRuntime1Mixin { self: Term =>
case (Closed(pnes1), Closed(pnes2)) => {
thisItself // When everything is closed, we only want thisItself and not the entire list of Closed possibilities
}
- case (Open(pnes1), Closed(pnes2)) => {
+ case (Open(pnes1), Closed(pnes2)) if !this.isInstanceOf[ChoiceGroupRef] => {
Review Comment:
This seems ok to me here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] jadams-tresys commented on a diff in pull request #967: Consider ChoiceGroupRef's open when determining next elements
Posted by "jadams-tresys (via GitHub)" <gi...@apache.org>.
jadams-tresys commented on code in PR #967:
URL: https://github.com/apache/daffodil/pull/967#discussion_r1113404377
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/TermRuntime1Mixin.scala:
##########
@@ -354,8 +355,12 @@ trait TermRuntime1Mixin { self: Term =>
case (Closed(pnes1), Closed(pnes2)) => {
thisItself // When everything is closed, we only want thisItself and not the entire list of Closed possibilities
}
- case (Open(pnes1), Closed(pnes2)) => {
+ case (Open(pnes1), Closed(pnes2)) if !this.isInstanceOf[ChoiceGroupRef] => {
Review Comment:
I can seen an argument to be made here that this logic should actually be part of SeveralPossibilitiesForNextElement in PartialNextElementResolver for when we are determining if the element is actually required or not. Figured I'd leave it up for discussion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a diff in pull request #967: Consider ChoiceGroupRef's open when determining next elements
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #967:
URL: https://github.com/apache/daffodil/pull/967#discussion_r1115742860
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/TermRuntime1Mixin.scala:
##########
@@ -354,8 +355,12 @@ trait TermRuntime1Mixin { self: Term =>
case (Closed(pnes1), Closed(pnes2)) => {
thisItself // When everything is closed, we only want thisItself and not the entire list of Closed possibilities
}
- case (Open(pnes1), Closed(pnes2)) => {
+ case (Open(pnes1), Closed(pnes2)) if !this.isInstanceOf[ChoiceGroupRef] => {
Review Comment:
I guess one benefit here is that this is checked at compile time instead of runtime? Seems like the preferred logic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org