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 2022/02/14 18:39:01 UTC
[GitHub] [daffodil] mbeckerle opened a new pull request #752: Add test cases for prefixed length bugs.
mbeckerle opened a new pull request #752:
URL: https://github.com/apache/daffodil/pull/752
DAFFODIL-2656, DAFFODIL-2657
--
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 change in pull request #752: Fix and add test cases for prefixed length bugs.
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #752:
URL: https://github.com/apache/daffodil/pull/752#discussion_r806794860
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/BinaryNumberTraits.scala
##########
@@ -108,15 +109,32 @@ trait PrefixedLengthParserMixin {
*/
def getPrefixedLengthInBits(state: PState): Long = {
val lenInUnits = getPrefixedLengthInUnits(state)
- lengthUnits match {
+ val lenInBits = lengthUnits match {
case LengthUnits.Bits => lenInUnits
case LengthUnits.Bytes => lenInUnits * 8
case LengthUnits.Characters => {
val mfw = state.encoder.bitsCharset.maybeFixedWidth
- if (mfw.isDefined) mfw.get
+ if (mfw.isDefined)
+ lenInUnits * mfw.get // fixed, only supported for parsing (maybe)
Review comment:
Should we add a test for this, codecov says there's no coverage.
Also, suggest this slight change to avoid the if-statement and make it I think a bit easier to read.
```scala
Assert.invariant(mfw.isDefined, "Prefixed length for text data in non-fixed width encoding.")
lenInUnits * mfw.get
```
--
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 pull request #752: Add test cases for prefixed length bugs.
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on pull request #752:
URL: https://github.com/apache/daffodil/pull/752#issuecomment-1039526100
@tuxji please re-review as I've now pushed the fixes also with more test coverage.
--
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 change in pull request #752: Fix and add test cases for prefixed length bugs.
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #752:
URL: https://github.com/apache/daffodil/pull/752#discussion_r806248453
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/BinaryNumberTraits.scala
##########
@@ -108,15 +109,32 @@ trait PrefixedLengthParserMixin {
*/
def getPrefixedLengthInBits(state: PState): Long = {
val lenInUnits = getPrefixedLengthInUnits(state)
- lengthUnits match {
+ val lenInBits = lengthUnits match {
case LengthUnits.Bits => lenInUnits
case LengthUnits.Bytes => lenInUnits * 8
case LengthUnits.Characters => {
val mfw = state.encoder.bitsCharset.maybeFixedWidth
- if (mfw.isDefined) mfw.get
+ if (mfw.isDefined)
+ lenInUnits * mfw.get // fixed, only supported for parsing (maybe)
else
Assert.invariantFailed("Prefixed length for text data in non-fixed width encoding.")
}
}
+ // Now we save the contentLength that we obtained from the prefix
+ // so that the dfdl:contentLength function can be called on the prefixed length element.
+ // as parse time.
Review comment:
Typo in the comment here. Assuming you don't want the period on the previous line and "at parse time" instead of "as parse time".
--
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 change in pull request #752: Fix and add test cases for prefixed length bugs.
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #752:
URL: https://github.com/apache/daffodil/pull/752#discussion_r806794860
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/BinaryNumberTraits.scala
##########
@@ -108,15 +109,32 @@ trait PrefixedLengthParserMixin {
*/
def getPrefixedLengthInBits(state: PState): Long = {
val lenInUnits = getPrefixedLengthInUnits(state)
- lengthUnits match {
+ val lenInBits = lengthUnits match {
case LengthUnits.Bits => lenInUnits
case LengthUnits.Bytes => lenInUnits * 8
case LengthUnits.Characters => {
val mfw = state.encoder.bitsCharset.maybeFixedWidth
- if (mfw.isDefined) mfw.get
+ if (mfw.isDefined)
+ lenInUnits * mfw.get // fixed, only supported for parsing (maybe)
Review comment:
Should we add a test for this, codecov says there's no coverage.
Also, suggest this slight change to avoid the if-statement and make it I think a bit easier to read.
```scala
Assert.invariant(mfw.isDefined, "Prefixed length for text data in non-fixed width encoding.")
lenInUnits * mfw.get
```
--
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 change in pull request #752: Fix and add test cases for prefixed length bugs.
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #752:
URL: https://github.com/apache/daffodil/pull/752#discussion_r806216222
##########
File path: daffodil-test/src/test/scala/org/apache/daffodil/section12/lengthKind/TestLengthKindPrefixed.scala
##########
@@ -150,4 +150,19 @@ class TestLengthKindPrefixed {
@Test def test_pl_lengthunits_err() = { runner.runOneTest("pl_lengthunits_err") }
@Test def test_pl_nest_err() = { runner.runOneTest("pl_nest_err") }
@Test def test_pl_decimal_err() = { runner.runOneTest("pl_decimal_err") }
+
+ // DAFFODIL-2657
+ @Test def test_pl_implicit_1() = { runner.runOneTest("pl_implicit_1")}
+
+ // DAFFODIL-2656
+ @Test def test_pl_complexContentLengthBytes_1() = { runner.runOneTest("pl_complexContentLengthBytes_1")}
+ @Test def test_pl_complexValueLengthBytes_1() = { runner.runOneTest("pl_complexValueLengthBytes_1")}
+ @Test def test_pl_complexContentLengthBits_1() = { runner.runOneTest("pl_complexContentLengthBits_1")}
+ @Test def test_pl_complexValueLengthBits_1() = { runner.runOneTest("pl_complexValueLengthBits_1")}
+ @Test def test_pl_simpleContentLengthBytes_1() = { runner.runOneTest("pl_simpleContentLengthBytes_1")}
+
+ // DAFFODIL-2658
+ @Test def test_pl_simpleValueLengthBytes_1() = { runner.runOneTest("pl_simpleValueLengthBytes_1")}
Review comment:
This test fails, must comment out.
--
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 change in pull request #752: Fix and add test cases for prefixed length bugs.
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #752:
URL: https://github.com/apache/daffodil/pull/752#discussion_r806216222
##########
File path: daffodil-test/src/test/scala/org/apache/daffodil/section12/lengthKind/TestLengthKindPrefixed.scala
##########
@@ -150,4 +150,19 @@ class TestLengthKindPrefixed {
@Test def test_pl_lengthunits_err() = { runner.runOneTest("pl_lengthunits_err") }
@Test def test_pl_nest_err() = { runner.runOneTest("pl_nest_err") }
@Test def test_pl_decimal_err() = { runner.runOneTest("pl_decimal_err") }
+
+ // DAFFODIL-2657
+ @Test def test_pl_implicit_1() = { runner.runOneTest("pl_implicit_1")}
+
+ // DAFFODIL-2656
+ @Test def test_pl_complexContentLengthBytes_1() = { runner.runOneTest("pl_complexContentLengthBytes_1")}
+ @Test def test_pl_complexValueLengthBytes_1() = { runner.runOneTest("pl_complexValueLengthBytes_1")}
+ @Test def test_pl_complexContentLengthBits_1() = { runner.runOneTest("pl_complexContentLengthBits_1")}
+ @Test def test_pl_complexValueLengthBits_1() = { runner.runOneTest("pl_complexValueLengthBits_1")}
+ @Test def test_pl_simpleContentLengthBytes_1() = { runner.runOneTest("pl_simpleContentLengthBytes_1")}
+
+ // DAFFODIL-2658
+ @Test def test_pl_simpleValueLengthBytes_1() = { runner.runOneTest("pl_simpleValueLengthBytes_1")}
Review comment:
This test fails, must comment out.
--
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 pull request #752: Add test cases for prefixed length bugs.
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on pull request #752:
URL: https://github.com/apache/daffodil/pull/752#issuecomment-1039526100
@tuxji please re-review as I've now pushed the fixes also with more test coverage.
--
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 merged pull request #752: Fix and add test cases for prefixed length bugs.
Posted by GitBox <gi...@apache.org>.
mbeckerle merged pull request #752:
URL: https://github.com/apache/daffodil/pull/752
--
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] tuxji commented on a change in pull request #752: Add test cases for prefixed length bugs.
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #752:
URL: https://github.com/apache/daffodil/pull/752#discussion_r806173852
##########
File path: daffodil-test/src/test/scala/org/apache/daffodil/section12/lengthKind/TestLengthKindPrefixed.scala
##########
@@ -150,4 +150,13 @@ class TestLengthKindPrefixed {
@Test def test_pl_lengthunits_err() = { runner.runOneTest("pl_lengthunits_err") }
@Test def test_pl_nest_err() = { runner.runOneTest("pl_nest_err") }
@Test def test_pl_decimal_err() = { runner.runOneTest("pl_decimal_err") }
+
+ // DAFFODIL-2657
+ // @Test
+ def test_pl_implicit_1() = { runner.runOneTest("pl_implicit_1")}
+
+ // DAFFODIL-2656
+ // @Test
+ def test_pl_contentLength_1() = { runner.runOneTest("pl_contentLength_1")}
Review comment:
Good idea to comment out only the @Test so the rest of the line still can be checked for compilation errors.
--
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 change in pull request #752: Fix and add test cases for prefixed length bugs.
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #752:
URL: https://github.com/apache/daffodil/pull/752#discussion_r806248453
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/BinaryNumberTraits.scala
##########
@@ -108,15 +109,32 @@ trait PrefixedLengthParserMixin {
*/
def getPrefixedLengthInBits(state: PState): Long = {
val lenInUnits = getPrefixedLengthInUnits(state)
- lengthUnits match {
+ val lenInBits = lengthUnits match {
case LengthUnits.Bits => lenInUnits
case LengthUnits.Bytes => lenInUnits * 8
case LengthUnits.Characters => {
val mfw = state.encoder.bitsCharset.maybeFixedWidth
- if (mfw.isDefined) mfw.get
+ if (mfw.isDefined)
+ lenInUnits * mfw.get // fixed, only supported for parsing (maybe)
else
Assert.invariantFailed("Prefixed length for text data in non-fixed width encoding.")
}
}
+ // Now we save the contentLength that we obtained from the prefix
+ // so that the dfdl:contentLength function can be called on the prefixed length element.
+ // as parse time.
Review comment:
Typo in the comment here. Assuming you don't want the period on the previous line and "at parse time" instead of "as parse time".
--
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] tuxji commented on a change in pull request #752: Add test cases for prefixed length bugs.
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #752:
URL: https://github.com/apache/daffodil/pull/752#discussion_r806173852
##########
File path: daffodil-test/src/test/scala/org/apache/daffodil/section12/lengthKind/TestLengthKindPrefixed.scala
##########
@@ -150,4 +150,13 @@ class TestLengthKindPrefixed {
@Test def test_pl_lengthunits_err() = { runner.runOneTest("pl_lengthunits_err") }
@Test def test_pl_nest_err() = { runner.runOneTest("pl_nest_err") }
@Test def test_pl_decimal_err() = { runner.runOneTest("pl_decimal_err") }
+
+ // DAFFODIL-2657
+ // @Test
+ def test_pl_implicit_1() = { runner.runOneTest("pl_implicit_1")}
+
+ // DAFFODIL-2656
+ // @Test
+ def test_pl_contentLength_1() = { runner.runOneTest("pl_contentLength_1")}
Review comment:
Good idea to comment out only the @Test so the rest of the line still can be checked for compilation errors.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetImpl.scala
##########
@@ -383,7 +383,7 @@ sealed abstract class LengthState(ie: DIElement) {
var maybeStartPos0bInBits: MaybeULong = MaybeULong.Nope
var maybeEndDataOutputStream: Maybe[DataOutputStream] = Nope
var maybeEndPos0bInBits: MaybeULong = MaybeULong.Nope
- var maybeComputedLength: MaybeULong = MaybeULong.Nope
+ var maybeComputedLength: MaybeULong = MaybeULong.Nope // Units are bits
Review comment:
Could rename `maybeComputedLength` to `maybeComputedLenInBits`, then wouldn't need this comment.
--
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] tuxji commented on a change in pull request #752: Fix and add test cases for prefixed length bugs.
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #752:
URL: https://github.com/apache/daffodil/pull/752#discussion_r806258973
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetImpl.scala
##########
@@ -383,7 +383,7 @@ sealed abstract class LengthState(ie: DIElement) {
var maybeStartPos0bInBits: MaybeULong = MaybeULong.Nope
var maybeEndDataOutputStream: Maybe[DataOutputStream] = Nope
var maybeEndPos0bInBits: MaybeULong = MaybeULong.Nope
- var maybeComputedLength: MaybeULong = MaybeULong.Nope
+ var maybeComputedLength: MaybeULong = MaybeULong.Nope // Units are bits
Review comment:
Could rename `maybeComputedLength` to `maybeComputedLenInBits`, then wouldn't need this comment.
--
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