You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by GitBox <gi...@apache.org> on 2020/01/02 19:30:58 UTC
[GitHub] [incubator-daffodil] stevedlawrence opened a new pull request #311:
Fix unparsing nested arrays when separatorSuppressionPolicy="never"
stevedlawrence opened a new pull request #311: Fix unparsing nested arrays when separatorSuppressionPolicy="never"
URL: https://github.com/apache/incubator-daffodil/pull/311
When data has separators and separatorSuppressionPolicy="never", then we
were not pushing/popping onto the arrayIndexStack during unparsing. This
meant that if an array with separatorSuppressionPolicy="never" was a
child of a parent array, then we would incorrectly modify the array
stack of the parent instead of the childs own array stack. This led to
incorrect array indices and invariant failures.
To fix this, this copies the array stack logic and invariant checks from
unparseWithSuppression into the unparseWithNoSuppression.
DAFFODIL-2263
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull
request #311: Fix unparsing nested arrays when
separatorSuppressionPolicy="never"
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #311: Fix unparsing nested arrays when separatorSuppressionPolicy="never"
URL: https://github.com/apache/incubator-daffodil/pull/311#discussion_r362614845
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/SeparatedSequenceUnparsers.scala
##########
@@ -260,7 +260,6 @@ class OrderedSeparatedSequenceUnparser(
val erd = unparser.erd
var numOccurrences = 0
val maxReps = unparser.maxRepeats(state)
- val isBounded = unparser.isBoundedMax
Review comment:
This is an interesting thing.
When refactoring this code I didn't create a constructor that required this as an argument. It is something needed by the general case, not by special cases. In some sense the special cases should still have all the parameters that the general case requires, or maybe should have them as Optional[Thing] and then just not use some of them. This falls in that category.
To me we lose something by deleting this line. The fact that isBounded isn't needed or used by this case, should somehow be explicitly stated. The classes as formulated don't force us to do this by providing None for an Option[Boolean].
I guess my suggestion is to leave this line in, but comment out this line and add a line-end comment that says unused in this case.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull
request #311: Fix unparsing nested arrays when
separatorSuppressionPolicy="never"
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #311: Fix unparsing nested arrays when separatorSuppressionPolicy="never"
URL: https://github.com/apache/incubator-daffodil/pull/311#discussion_r362605933
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/SeparatedSequenceUnparsers.scala
##########
@@ -260,7 +260,6 @@ class OrderedSeparatedSequenceUnparser(
val erd = unparser.erd
var numOccurrences = 0
val maxReps = unparser.maxRepeats(state)
- val isBounded = unparser.isBoundedMax
Review comment:
Could be a bug that it is unused.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence merged pull request #311: Fix
unparsing nested arrays when separatorSuppressionPolicy="never"
Posted by GitBox <gi...@apache.org>.
stevedlawrence merged pull request #311: Fix unparsing nested arrays when separatorSuppressionPolicy="never"
URL: https://github.com/apache/incubator-daffodil/pull/311
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull
request #311: Fix unparsing nested arrays when
separatorSuppressionPolicy="never"
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #311: Fix unparsing nested arrays when separatorSuppressionPolicy="never"
URL: https://github.com/apache/incubator-daffodil/pull/311#discussion_r362608337
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/SeparatedSequenceUnparsers.scala
##########
@@ -260,7 +260,6 @@ class OrderedSeparatedSequenceUnparser(
val erd = unparser.erd
var numOccurrences = 0
val maxReps = unparser.maxRepeats(state)
- val isBounded = unparser.isBoundedMax
Review comment:
It loosk like ``isBounded`` is only used in ``unparsePositionallyRequiredSeps``, which is only called from ``unparseWithSuppression``. I'm thinking that because ``separatorSuppressionPolicy="never"`` that there is no need for positionally required separators, and no need to check if the occurrences are bounded? Note that ``unparseWithNoSuppression`` does look at the ``maxReps`` to determine if extra separators are needed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull
request #311: Fix unparsing nested arrays when
separatorSuppressionPolicy="never"
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #311: Fix unparsing nested arrays when separatorSuppressionPolicy="never"
URL: https://github.com/apache/incubator-daffodil/pull/311#discussion_r362793425
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/SeparatedSequenceUnparsers.scala
##########
@@ -260,7 +260,6 @@ class OrderedSeparatedSequenceUnparser(
val erd = unparser.erd
var numOccurrences = 0
val maxReps = unparser.maxRepeats(state)
- val isBounded = unparser.isBoundedMax
Review comment:
Looking at the again, the ``val isBounded`` that is unused and removed above was actualy from the ``unparseWithSuppression`` case, which does end up using it when it calls ``unparsePositionallyRequiredSeps``, it's just not passed in.
So probably what happened is that ``isBounded`` was used at some point in this function, but that logic got refactored out to a function, and the variable just wasn't removed.
I do agree that it's probably still valuable to point out that isBounded is not used in the no-suppression case, so I'll add that and comment it out. But I think this line above can stay removed, since isBounded does apply, just via another function.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull
request #311: Fix unparsing nested arrays when
separatorSuppressionPolicy="never"
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #311: Fix unparsing nested arrays when separatorSuppressionPolicy="never"
URL: https://github.com/apache/incubator-daffodil/pull/311#discussion_r362599108
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/SeparatedSequenceUnparsers.scala
##########
@@ -260,7 +260,6 @@ class OrderedSeparatedSequenceUnparser(
val erd = unparser.erd
var numOccurrences = 0
val maxReps = unparser.maxRepeats(state)
- val isBounded = unparser.isBoundedMax
Review comment:
Notice this variable was never used. Unrelate to the fix, but thought I'd clean this up and remove it while I'm 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services