stevedlawrence commented on code in PR #1338:
URL: https://github.com/apache/daffodil/pull/1338#discussion_r1808993978
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SpecifiedLengthParsers.scala:
##########
@@ -85,8 +85,8 @@ sealed abstract class SpecifiedLengthParserBase(eParser:
Parser, erd: RuntimeDat
if (pState.processorStatus ne Success) return
val finalEndPos0b = startingBitPos0b + nBits
- // if we haven't already set the value length, set it now
- if (pState.infoset.valueLength.isEndUndef)
+ // we want to capture the length before we do any skipping
Review Comment:
Might be worth adding a comment that value length of simple types is
captured by the eParser if needed.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/ElementBaseRuntime1Mixin.scala:
##########
@@ -89,17 +89,19 @@ trait ElementBaseRuntime1Mixin { self: ElementBase =>
//
// For complex elements with specified length, value length is captured in
// the specified length parsers, since they handle skipping unused
- // element regions. For complex elements, this means lengthKind is not
+ // element regions, and the length capturing has to be done before that.
+ // For complex elements, this means lengthKind is not
// implicit or delimited.
//
// So for these cases we do not want to capture value length with the
// Capture{Start,End}OfValueLengthParsers, since those lengths are captured
// by the value parsers
- val capturedByParsers =
- (isSimpleType && (impliedRepresentation == Representation.Text ||
lengthKind == LengthKind.Delimited)) ||
+ val capturedByValueParsers =
+ (isSimpleType && (
+ primType == PrimType.String || lengthKind == LengthKind.Delimited)) ||
Review Comment:
I'm wondering if this change is correct?
For example, say we have this element:
```xml
<xs:element name="foo" type="xs:int" dfdl:representation="text"
dfdl:trimKind="padChar" dfdl:lengthKind="explicit" dfdl:length="10" ... />
```
So a fixed length text integer with padding. In this case I think what
Daffodil does is it create a String parser to parse the fixed length string and
remove padding, and then creates another parser to convert that string to an
actual integer.
So in that case, even though the primType is not String, I think the String
parser will still be used to capture the value length after padding is removed.
So I *think* `impliedRepresentation == Text` is still needed?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/ElementBaseRuntime1Mixin.scala:
##########
@@ -24,9 +24,9 @@ import org.apache.daffodil.core.dsom.PrimitiveType
import org.apache.daffodil.core.dsom.Root
import org.apache.daffodil.core.dsom.SimpleTypeDefBase
Review Comment:
I wonder if we should modify `setAbsStartBitPos0bInBits` so it does
`Assert.invariant(maybeStartPos0bInBits.isEmpty)` and something similar for
`setAbsEndBitPos0bInBits`? I think those assertion would have caught this bug
since if it fails it means we have set the absolute bit position twice, which
we should never do.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
