[GitHub] [daffodil] tuxji commented on a diff in pull request #1015: Fix separatorSuppressionPolicy="never"

["tuxji (via GitHub)" <gi...@apache.org>]

tuxji commented on code in PR #1015:
URL: https://github.com/apache/daffodil/pull/1015#discussion_r1206003308


##########
daffodil-test/src/test/resources/org/apache/daffodil/usertests/SepTests.tdml:
##########
@@ -223,7 +223,7 @@
 
   <!-- Test for DAFFODIL-2499. empty strings should not be creating empty elements here. -->
   <tdml:parserTestCase name="test_sep_ssp_never_1" root="file1" model="s3"
-                       implementations="ibm">
+                       implementations="daffodil ibm">
     <!--
       This test doesn't work on Daffodil due  to DAFFODIL-2499.

Review Comment:
   Since you've enabled this test, this test must be working on Daffodil now so you should remove or change this comment saying the test doesn't work on Daffodil.



##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/SeparatedSequenceUnparsers.scala:
##########
@@ -612,120 +612,96 @@ class OrderedSeparatedSequenceUnparser(
           val erd = unparser.erd
           var numOccurrences = 0
           val maxReps = unparser.maxRepeats(state)
-          // val isBounded = unparser.isBoundedMax // not needed for the no-suppression case
 
-          //
-          // The number of occurrances we unparse is always exactly driven
-          // by the number of infoset events for the repeating/optional element.
-          //
-          // For RepUnparser - array/optional case - in all cases we should get a
-          // startArray event. If we don't then
-          // the element must be entirely optional, so we get no events for it
-          // at all.
-          //
+          Assert.invariant(state.inspect)

Review Comment:
   It's hard for me to find the else branch that went away in this diff.  This function is just too long and complicated to make it easy to understand how the logic has changed only from looking at the diff.  Oh well, please add a comment at least explaining why state.inspect=true is an invariant now when it wasn't an invariant on this code path before.



##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SeparatedSequenceChildParseResultHelper.scala:
##########
@@ -94,7 +95,30 @@ trait SeparatedSequenceChildParseResultHelper extends SequenceChildParseResultHe
    * Define this as final here so we aren't creating proliferation of
    * traits/classes just for this one little issue.

Review Comment:
   How does defining a final method avoid the need for more than one trait?



-- 
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