stevedlawrence commented on a change in pull request #371:
URL: https://github.com/apache/incubator-daffodil/pull/371#discussion_r415774331



##########
File path: 
daffodil-core/src/main/scala/org/apache/daffodil/grammar/primitives/SequenceChild.scala
##########
@@ -123,7 +123,7 @@ abstract class SequenceChild(protected val sq: 
SequenceTermBase, child: Term, gr
     import SeparatorSuppressionPolicy._
     import SeparatedSequenceChildBehavior._
     child match {
-      case m: ModelGroup => {
+      case ch @  (_: ModelGroup | _: ElementBase) if 
ch.isInstanceOf[ModelGroup] || ch.isScalar => {

Review comment:
       I personally find this case more difficult to parse out. Maybe move the 
content of these two cases in a lazy val or def and referece it in the cases?

##########
File path: 
daffodil-runtime1/src/main/scala/org/apache/daffodil/layers/LineFoldedTransformer.scala
##########
@@ -296,10 +296,6 @@ class LineFoldedInputStream(mode: LineFoldMode, jis: 
InputStream)
         case GotCRLF => {
           c = jis.read()
           c match {
-            case -1 => {
-              state = Buf2 // buffering up the LF
-              return '\r'
-            }

Review comment:
       Although the code here is the same as the default case at the end, the 
comment is different. Can you adjust the comment of the default case so it 
mentions what this case is checking for?, something like "CRLF followed by 
other not sp/tab, or end of data. Buffer up to the LF" 




----------------------------------------------------------------
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:
[email protected]


Reply via email to