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/03/10 21:33:21 UTC
[GitHub] [daffodil] mbeckerle opened a new pull request #770: Added dfdlx:alignmentViaSkip property
mbeckerle opened a new pull request #770:
URL: https://github.com/apache/daffodil/pull/770
DAFFODIL-2652
--
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 #770: Added dfdlx:alignmentViaSkip property
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #770:
URL: https://github.com/apache/daffodil/pull/770#discussion_r825883705
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/dfa/TextParser.scala
##########
@@ -54,7 +54,7 @@ class TextParser(override val context: TermRuntimeData)
if (isDelimRequired) Nope
else {
val totalNumCharsRead = 0
- input.getString(totalNumCharsRead, state)
+ input.getSomeString(totalNumCharsRead, state)
Review comment:
This behavior is slightly different, this now doesn't care if it doesn't get totalNumCharsRead--before it would have been an error. Maybe that wasn't possible in this code path? It's hard to tell.
Also a bit concerning that this change has no test coverage. Maybe this code isn't even possible to reach?
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/ParseErrors.scala
##########
@@ -39,6 +40,14 @@ class ParseError(rd: Maybe[SchemaFileLocation], val loc: Maybe[DataLocation], ca
override def toParseError = this
}
+final class CharsetNotByteAlignedError(pstate: PState,
+ cause: BitsCharsetDecoderUnalignedCharDecodeException)
+ extends ParseError(
Review comment:
Should this be a schema definition error? The only way this *should* be possible if is someone sets alignmentKind to manual but their schema isn't written correctly for manual alignment. So it's really a sign of a bad schema and maybe we don't want the chance to recover via backtracking?
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/grammar/AlignedMixin.scala
##########
@@ -48,13 +51,29 @@ trait AlignedMixin extends GrammarMixin { self: Term =>
requiredEvaluationsIfActivated(hasNoSkipRegions)
+ /**
+ * If "manual" this property disables all automatic alignment. The
+ * schema author must use leadingSkip, trailingSkip, or just ensure
+ * all the elements/terms are alignmed based on their length.
+ */
+ lazy val alignmentKind: AlignmentKind = {
+ val prop = findPropertyOption("alignmentKind")
+ prop match {
+ case NotFound(_, _, _) => AlignmentKind.Automatic // default since this is a new property
+ case Found("automatic",_,_,_) => AlignmentKind.Automatic
+ case Found("manual",_,_,_) => AlignmentKind.Manual
+ case _ => Assert.impossible("Per validation, can only be 'automatic' or 'manual'")
+ }
Review comment:
Agreed.
A second thought, why are we even manually implementing this? Shouldn't the property generator be able to easily generate code for this? Or does that not handle extension properties or something?
I guess we'd still need something manual to provide a default, but this could look something like
```scala
lazy val alignmentKindDefaulted = optionAlignmentKind.getOrElse(AlignmentKind.Automatic)
```
I think we use a similar pattern for other properties that have a default value?
--
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 #770: Added dfdlx:alignmentViaSkip property
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #770:
URL: https://github.com/apache/daffodil/pull/770#discussion_r824815818
##########
File path: daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/dfdlx.xsd
##########
@@ -47,6 +48,27 @@
</xs:restriction>
</xs:simpleType>
+
+ <xs:attribute name="alignmentViaSkip" type="dfdl:YesNoEnum" default="no">
Review comment:
Thoughts on changing this to something like "alignmentPolicy"? I've mentioned other tweaks to deal with the alignment optimizations, but that are more complicated to implement. It might be nice to have a single property that essentially lets us choose between different alginment optimizations. And they could vary from a very pessimistic algorithm like we have now, or this optimistic one that completely disables alignment like this, or somewhere in between? We'd only have two values for now, so funtionally the same as a yes/no choice, but we could expand it in the future.
--
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 #770: Added dfdlx:alignmentViaSkip property
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #770:
URL: https://github.com/apache/daffodil/pull/770#discussion_r825188438
##########
File path: daffodil-io/src/main/scala/org/apache/daffodil/io/InputSourceDataInputStream.scala
##########
@@ -614,40 +614,44 @@ final class InputSourceDataInputStream private(val inputSource: InputSource)
markPool.finalCheck
}
+ /**
+ * Returns exactly One nChars-long string, or Nope.
+ *
+ * @param nChars
+ * @param finfo
+ * @return
+ */
final def getString(nChars: Long, finfo: FormatInfo): Maybe[String] = {
val startingBitPos = bitPos0b
- val aligned = align(finfo.encodingMandatoryAlignmentInBits, finfo)
- if (!aligned) {
- Maybe.Nope
- } else {
- withLocalCharBuffer { lcb =>
- val cb = lcb.getBuf(nChars)
- val numDecoded = finfo.decoder.decode(this, finfo, cb)
- if (numDecoded == nChars) {
- Maybe(cb.flip.toString)
- } else {
- setBitPos0b(startingBitPos)
- Maybe.Nope
- }
+ withLocalCharBuffer { lcb =>
+ val cb = lcb.getBuf(nChars)
+ val numDecoded = finfo.decoder.decode(this, finfo, cb)
+ if (numDecoded == nChars) {
+ Maybe(cb.flip.toString)
+ } else {
+ setBitPos0b(startingBitPos)
+ Maybe.Nope
}
Review comment:
I'm going to consolidate, and just have getSomeString().
--
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 #770: Added dfdlx:alignmentViaSkip property
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #770:
URL: https://github.com/apache/daffodil/pull/770#discussion_r825453914
##########
File path: daffodil-io/src/main/scala/org/apache/daffodil/processors/charset/BitsCharsetDecoder.scala
##########
@@ -32,6 +31,16 @@ import org.apache.daffodil.util.MaybeChar
class BitsCharsetDecoderMalformedException(val malformedBits: Int)
extends ThinException
+class BitsCharsetDecoderUnalignedCharDecodeException(val bitPos1b: Long)
+ extends ThinException {
+ def bitAlignment1b = bitPos1b % 8
+ def bytePos1b = ((bitPos1b - 1) / 8) + 1
+
+ override def getMessage(): String = {
+ s"Charset not byte aligned. bitAlignment1b=${bitAlignment1b}, bitPos1b=${bitPos1b}, bytePos1b=${bytePos1b}."
Review comment:
I like the change!
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/grammar/AlignedMixin.scala
##########
@@ -48,13 +51,29 @@ trait AlignedMixin extends GrammarMixin { self: Term =>
requiredEvaluationsIfActivated(hasNoSkipRegions)
+ /**
+ * If "manual" this property disables all automatic alignment. The
+ * schema author must use leadingSkip, trailingSkip, or just ensure
+ * all the elements/terms are alignmed based on their length.
+ */
+ lazy val alignmentKind: AlignmentKind = {
+ val prop = findPropertyOption("alignmentKind")
+ prop match {
+ case NotFound(_, _, _) => AlignmentKind.Automatic // default since this is a new property
+ case Found("automatic",_,_,_) => AlignmentKind.Automatic
+ case Found("manual",_,_,_) => AlignmentKind.Manual
+ case _ => Assert.impossible("Per validation, can only be 'automatic' or 'manual'")
+ }
Review comment:
Much more readable now! Even covers the impossible case so readers will understand what's happening.
##########
File path: daffodil-io/src/main/scala/org/apache/daffodil/io/InputSourceDataInputStream.scala
##########
@@ -614,40 +614,23 @@ final class InputSourceDataInputStream private(val inputSource: InputSource)
markPool.finalCheck
}
- final def getString(nChars: Long, finfo: FormatInfo): Maybe[String] = {
- val startingBitPos = bitPos0b
- val aligned = align(finfo.encodingMandatoryAlignmentInBits, finfo)
- if (!aligned) {
- Maybe.Nope
- } else {
- withLocalCharBuffer { lcb =>
- val cb = lcb.getBuf(nChars)
- val numDecoded = finfo.decoder.decode(this, finfo, cb)
- if (numDecoded == nChars) {
- Maybe(cb.flip.toString)
- } else {
- setBitPos0b(startingBitPos)
- Maybe.Nope
- }
- }
- }
- }
-
+ /**
+ * Almost the same as getString. But returns some characters (up to nChars) if they are available.
+ *
+ * @param nChars From 1 up to this many characters are returned as the string, or Nope for 0.
+ * @param finfo
+ * @return
+ */
final def getSomeString(nChars: Long, finfo: FormatInfo): Maybe[String] = {
val startingBitPos = bitPos0b
- val aligned = align(finfo.encodingMandatoryAlignmentInBits, finfo)
- if (!aligned) {
- Maybe.Nope
- } else {
- withLocalCharBuffer { lcb =>
- val cb = lcb.getBuf(nChars)
- val numDecoded = finfo.decoder.decode(this, finfo, cb)
- if (numDecoded > 0) {
- Maybe(cb.flip.toString)
- } else {
- setBitPos0b(startingBitPos)
- Maybe.Nope
- }
+ withLocalCharBuffer { lcb =>
+ val cb = lcb.getBuf(nChars)
+ val numDecoded = finfo.decoder.decode(this, finfo, cb)
+ if (numDecoded > 0) { // This is the difference with the above getString
Review comment:
This comment should be removed too, yes?
##########
File path: daffodil-io/src/main/scala/org/apache/daffodil/io/InputSourceDataInputStream.scala
##########
@@ -614,40 +614,23 @@ final class InputSourceDataInputStream private(val inputSource: InputSource)
markPool.finalCheck
}
- final def getString(nChars: Long, finfo: FormatInfo): Maybe[String] = {
- val startingBitPos = bitPos0b
- val aligned = align(finfo.encodingMandatoryAlignmentInBits, finfo)
- if (!aligned) {
- Maybe.Nope
- } else {
- withLocalCharBuffer { lcb =>
- val cb = lcb.getBuf(nChars)
- val numDecoded = finfo.decoder.decode(this, finfo, cb)
- if (numDecoded == nChars) {
- Maybe(cb.flip.toString)
- } else {
- setBitPos0b(startingBitPos)
- Maybe.Nope
- }
- }
- }
- }
-
+ /**
+ * Almost the same as getString. But returns some characters (up to nChars) if they are available.
Review comment:
I believe you've removed getString so this comment should just say "Returns some...", yes?
--
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 #770: Added dfdlx:alignmentViaSkip property
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #770:
URL: https://github.com/apache/daffodil/pull/770#discussion_r824886932
##########
File path: daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/dfdlx.xsd
##########
@@ -47,6 +48,27 @@
</xs:restriction>
</xs:simpleType>
+
+ <xs:attribute name="alignmentViaSkip" type="dfdl:YesNoEnum" default="no">
Review comment:
I was thinking alignmentKind, by direct analogy to lengthKind.
Given that we have alignmentUnits and lengthUnits I think using the same suffix as length is perhaps the pattern to follow.
--
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 #770: Added dfdlx:alignmentViaSkip property
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #770:
URL: https://github.com/apache/daffodil/pull/770#discussion_r824235837
##########
File path: daffodil-io/src/main/scala/org/apache/daffodil/processors/charset/BitsCharset.scala
##########
@@ -55,7 +55,7 @@ trait BitsCharset extends Serializable {
def aliases: Seq[String] = Nil
def bitWidthOfACodeUnit: Int // in units of bits
def requiredBitOrder: BitOrder
- def mandatoryBitAlignment: Int
+ def mandatoryBitAlignment: Int // ignored when dfdlx:alignmentViaSkip is "yes"
Review comment:
I agree, let's add a test to determine whether the diagnostic looks appropriate.
--
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 #770: Added dfdlx:alignmentViaSkip property
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #770:
URL: https://github.com/apache/daffodil/pull/770#discussion_r825175804
##########
File path: daffodil-io/src/main/scala/org/apache/daffodil/processors/charset/BitsCharsetDecoder.scala
##########
@@ -32,6 +31,13 @@ import org.apache.daffodil.util.MaybeChar
class BitsCharsetDecoderMalformedException(val malformedBits: Int)
extends ThinException
+class BitsCharsetDecoderUnalignedCharDecodeException(val bitAlignment1b: Long)
+ extends ThinException {
+ override def getMessage(): String = {
+ s"Charset not byte aligned. Bit alignment (1b): ${bitAlignment1b}."
Review comment:
Arguably we should save the bitPos1b and have the message refer to both the bitAlignment1b, the bitPos1b and the bytePos1b since it depends on the data what is useful to the user.
I will improve this.
--
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 #770: Added dfdlx:alignmentViaSkip property
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #770:
URL: https://github.com/apache/daffodil/pull/770#discussion_r825307544
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/grammar/AlignedMixin.scala
##########
@@ -48,13 +50,27 @@ trait AlignedMixin extends GrammarMixin { self: Term =>
requiredEvaluationsIfActivated(hasNoSkipRegions)
+ /**
+ * If "manual" this property disables all automatic alignment. The
+ * schema author must use leadingSkip, trailingSkip, or just ensure
+ * all the elements/terms are alignmed based on their length.
+ */
+ lazy val alignmentKind: AlignmentKind = {
+ val prop = findPropertyOption("alignmentKind")
+ prop match {
+ case NotFound(_, _, _) => AlignmentKind.Automatic // default
+ case _ => AlignmentKind.Manual
+ }
Review comment:
I think @tuxji is right, I think there is still a logic bug. If the property is defined this ignores the value and sets the kind to manual. So even if you do `dfdlx:alignmentKind="automatic"` you still get manual behavior? We should add a test that `dfdlx:alignmentKind="automatic"` works.
--
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 #770: Added dfdlx:alignmentViaSkip property
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #770:
URL: https://github.com/apache/daffodil/pull/770#discussion_r824177252
##########
File path: daffodil-io/src/main/scala/org/apache/daffodil/processors/charset/BitsCharset.scala
##########
@@ -55,7 +55,7 @@ trait BitsCharset extends Serializable {
def aliases: Seq[String] = Nil
def bitWidthOfACodeUnit: Int // in units of bits
def requiredBitOrder: BitOrder
- def mandatoryBitAlignment: Int
+ def mandatoryBitAlignment: Int // ignored when dfdlx:alignmentViaSkip is "yes"
Review comment:
I think we need a test to see what happens if we're using regular 8-bit ascii charset encoding, but it isn't 8-bit aligned because we forced it to not do alignment via dfdlx:alignmentViaSkip. Similarly for utf-16 that isn't byte aligned.
Getting a runtime error here, so long as the diagnostic indicates the character is not aligned properly, is the right behavior we want.
--
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 #770: Added dfdlx:alignmentViaSkip property
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #770:
URL: https://github.com/apache/daffodil/pull/770#discussion_r824989379
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/grammar/AlignedMixin.scala
##########
@@ -48,13 +50,27 @@ trait AlignedMixin extends GrammarMixin { self: Term =>
requiredEvaluationsIfActivated(hasNoSkipRegions)
+ /**
+ * If "manual" this property disables all automatic alignment. The
+ * schema author must use leadingSkip, trailingSkip, or just ensure
+ * all the elements/terms are alignmed based on their length.
+ */
+ lazy val alignmentKind: AlignmentKind = {
+ val prop = findPropertyOption("alignmentKind")
+ prop match {
+ case NotFound(_, _, _) => AlignmentKind.Automatic // default
+ case _ => AlignmentKind.Manual
+ }
Review comment:
Does this match expression handle `dfdlx:alignmentKind="automatic"` as well as `dfdlx:alignmentKind="manual"` correctly? I think you need a test with `dfdlx:alignmentKind="automatic"` to test this match expression expansively and perhaps also a negative test with `dfdlx:alignmentKind="misspelling"` to check that Daffodil emits an appropriate error message too.
##########
File path: daffodil-io/src/main/scala/org/apache/daffodil/io/InputSourceDataInputStream.scala
##########
@@ -614,40 +614,44 @@ final class InputSourceDataInputStream private(val inputSource: InputSource)
markPool.finalCheck
}
+ /**
+ * Returns exactly One nChars-long string, or Nope.
+ *
+ * @param nChars
+ * @param finfo
+ * @return
+ */
final def getString(nChars: Long, finfo: FormatInfo): Maybe[String] = {
val startingBitPos = bitPos0b
- val aligned = align(finfo.encodingMandatoryAlignmentInBits, finfo)
- if (!aligned) {
- Maybe.Nope
- } else {
- withLocalCharBuffer { lcb =>
- val cb = lcb.getBuf(nChars)
- val numDecoded = finfo.decoder.decode(this, finfo, cb)
- if (numDecoded == nChars) {
- Maybe(cb.flip.toString)
- } else {
- setBitPos0b(startingBitPos)
- Maybe.Nope
- }
+ withLocalCharBuffer { lcb =>
+ val cb = lcb.getBuf(nChars)
+ val numDecoded = finfo.decoder.decode(this, finfo, cb)
+ if (numDecoded == nChars) {
+ Maybe(cb.flip.toString)
+ } else {
+ setBitPos0b(startingBitPos)
+ Maybe.Nope
}
Review comment:
Code coverage says those lines are never called. I looked for callers of `getString(nChars, finfo)` and the only place I found seems to be one line in [TextParser.scala](https://github.com/apache/daffodil/blob/55f638de74edfa400ce6cba1de19162069c9298a/daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/dfa/TextParser.scala#L57) which completely ignores the return value and doesn't check if Maybe.Nope is returned. The new tests added below only call `getSomeString(nChars, finfo)`, not `getString(nChars, finfo)`.
##########
File path: daffodil-io/src/main/scala/org/apache/daffodil/processors/charset/BitsCharsetDecoder.scala
##########
@@ -32,6 +31,13 @@ import org.apache.daffodil.util.MaybeChar
class BitsCharsetDecoderMalformedException(val malformedBits: Int)
extends ThinException
+class BitsCharsetDecoderUnalignedCharDecodeException(val bitAlignment1b: Long)
+ extends ThinException {
+ override def getMessage(): String = {
+ s"Charset not byte aligned. Bit alignment (1b): ${bitAlignment1b}."
Review comment:
Is reporting the desired `bitAlignment1b` more useful for debugging than reporting the current value of the stream's `bitPos1b`? Maybe the caller on line 141 should pass `dis.bitPos1b` instead of `dis.bitPos1b % 8` and this exception should capture the value as `bitPos1b` instead of `bitAlignment1b`?
##########
File path: daffodil-test/src/test/resources/org/apache/daffodil/section10/representation_properties/encodings.tdml
##########
@@ -256,7 +261,63 @@
</tdml:dfdlInfoset>
</tdml:infoset>
</tdml:parserTestCase>
-
-
-
+
+
+ <tdml:defineSchema
+ name="unalignedText"
+ elementFormDefault="unqualified"
+ useDefaultNamespace="false">
+
+ <xs:include schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+
+ <dfdl:format ref="tns:GeneralFormat"
+ lengthKind="explicit"
+ representation="binary"
+ lengthUnits="bits"
+ alignmentUnits="bits"
+ dfdlx:alignmentKind="manual"/>
+
+ <xs:element name="r" dfdl:lengthKind="implicit">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:element name="fourBits" type="xs:unsignedInt" dfdl:lengthUnits="bits" dfdl:length="4"/>
+ <!--
+ This next element is a string, ASCII, which requires 8-bit mandatory alignment,
+ but the dfdlx:alignmentKind 'manual' turns off automatic alignment.
+ So this parse will try to start not at a byte boundary.
+
+ However, the decoder itself will move over to a byte boundary when that is required
+ by the decoder.
+ -->
+ <xs:element name="str" type="xs:string" dfdl:lengthUnits="bytes" dfdl:length="3"/>
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
+
+ </tdml:defineSchema>
+
+ <tdml:parserTestCase
+ name="unalignedCharsetWithMandatory8BitAlignment"
+ model="unalignedText"
+ root="r"
+ description="Not aligned when we start parsing a charset where the decoder requires 8-bit alignment">
+ <tdml:document>
+ <tdml:documentPart type="byte">
+ F3 34 35 36
+ </tdml:documentPart>
+ </tdml:document>
+<!-- <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <ex:r>
+ <fourBits>15</fourBits>
+ <str>345</str>
+ </ex:r>
+ </tdml:dfdlInfoset>
+ </tdml:infoset>-->
Review comment:
Is this dead code that should be removed?
##########
File path: daffodil-test/src/test/scala/org/apache/daffodil/section17/calc_value_properties/TestComputedLengthFields.scala
##########
@@ -39,13 +39,24 @@ class TestComputedLengthFields {
@Test def test_computedLengthAroundPrefixedLengths1p(): Unit = { runner.runOneTest("computedLengthAroundPrefixedLengths1p") }
// DAFFODIL-2626 - deadlock interaction between computed length and prefixed-length strings.
- // @Test def test_computedLengthAroundPrefixedLengths1u(): Unit = { runner.runOneTest("computedLengthAroundPrefixedLengths1u") }
+ // @Test
+ def test_computedLengthAroundPrefixedLengths1u(): Unit = { runner.runOneTest("computedLengthAroundPrefixedLengths1u") }
+
+ // This test shows you can work around DAFFODIL-2626 using the dfdlx:alignmentKine='manual' property.
Review comment:
typo: `dfdlx:alignmentKind`
--
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 #770: Added dfdlx:alignmentViaSkip property
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #770:
URL: https://github.com/apache/daffodil/pull/770#discussion_r825175422
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/grammar/AlignedMixin.scala
##########
@@ -48,13 +50,27 @@ trait AlignedMixin extends GrammarMixin { self: Term =>
requiredEvaluationsIfActivated(hasNoSkipRegions)
+ /**
+ * If "manual" this property disables all automatic alignment. The
+ * schema author must use leadingSkip, trailingSkip, or just ensure
+ * all the elements/terms are alignmed based on their length.
+ */
+ lazy val alignmentKind: AlignmentKind = {
+ val prop = findPropertyOption("alignmentKind")
+ prop match {
+ case NotFound(_, _, _) => AlignmentKind.Automatic // default
+ case _ => AlignmentKind.Manual
+ }
Review comment:
All that sort of defensive programming and testing for properties is handled centrally by XML validation of the DFDL schemas. Those validators for short-form props are generated off of the XML schemas for properties and extension properties.
This is a big advantage of XML over other things without robust schema languages like JSON.
--
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 #770: Added dfdlx:alignmentViaSkip property
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #770:
URL: https://github.com/apache/daffodil/pull/770#discussion_r825998291
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/dfa/TextParser.scala
##########
@@ -54,7 +54,7 @@ class TextParser(override val context: TermRuntimeData)
if (isDelimRequired) Nope
else {
val totalNumCharsRead = 0
- input.getString(totalNumCharsRead, state)
+ input.getSomeString(totalNumCharsRead, state)
Review comment:
I do think it is not reachable any more. I will look at it again, and if it's really dead code I'll remove.
--
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 #770: Added dfdlx:alignmentViaSkip property
Posted by GitBox <gi...@apache.org>.
mbeckerle merged pull request #770:
URL: https://github.com/apache/daffodil/pull/770
--
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 #770: Added dfdlx:alignmentViaSkip property
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #770:
URL: https://github.com/apache/daffodil/pull/770#discussion_r826000800
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/ParseErrors.scala
##########
@@ -39,6 +40,14 @@ class ParseError(rd: Maybe[SchemaFileLocation], val loc: Maybe[DataLocation], ca
override def toParseError = this
}
+final class CharsetNotByteAlignedError(pstate: PState,
+ cause: BitsCharsetDecoderUnalignedCharDecodeException)
+ extends ParseError(
Review comment:
This is a tricky point. I went with Parse error because the notion is that a schema could be speculating down a branch where the alignment doesn't work out, and this error allows it to backtrack to the proper branch where it does. This is very analogous to speculating down a branch where decoding characters causes failures or delimiters aren't found properly.
--
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 #770: Added dfdlx:alignmentViaSkip property
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #770:
URL: https://github.com/apache/daffodil/pull/770#discussion_r825313009
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/grammar/AlignedMixin.scala
##########
@@ -48,13 +50,27 @@ trait AlignedMixin extends GrammarMixin { self: Term =>
requiredEvaluationsIfActivated(hasNoSkipRegions)
+ /**
+ * If "manual" this property disables all automatic alignment. The
+ * schema author must use leadingSkip, trailingSkip, or just ensure
+ * all the elements/terms are alignmed based on their length.
+ */
+ lazy val alignmentKind: AlignmentKind = {
+ val prop = findPropertyOption("alignmentKind")
+ prop match {
+ case NotFound(_, _, _) => AlignmentKind.Automatic // default
+ case _ => AlignmentKind.Manual
+ }
Review comment:
Yup. Upon re-reading, haste makes waste. I will fix this and add a test.
--
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 #770: Added dfdlx:alignmentViaSkip property
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #770:
URL: https://github.com/apache/daffodil/pull/770#discussion_r825995961
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/grammar/AlignedMixin.scala
##########
@@ -48,13 +51,29 @@ trait AlignedMixin extends GrammarMixin { self: Term =>
requiredEvaluationsIfActivated(hasNoSkipRegions)
+ /**
+ * If "manual" this property disables all automatic alignment. The
+ * schema author must use leadingSkip, trailingSkip, or just ensure
+ * all the elements/terms are alignmed based on their length.
+ */
+ lazy val alignmentKind: AlignmentKind = {
+ val prop = findPropertyOption("alignmentKind")
+ prop match {
+ case NotFound(_, _, _) => AlignmentKind.Automatic // default since this is a new property
+ case Found("automatic",_,_,_) => AlignmentKind.Automatic
+ case Found("manual",_,_,_) => AlignmentKind.Manual
+ case _ => Assert.impossible("Per validation, can only be 'automatic' or 'manual'")
+ }
Review comment:
You know, that may be the case. I may have over complicated this. This is a regular property, except extension namespace, and default value.
--
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