stevedlawrence commented on code in PR #1338:
URL: https://github.com/apache/daffodil/pull/1338#discussion_r1840490014
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -1387,14 +1381,15 @@ trait ElementBaseGrammarMixin
}
case LengthKind.Implicit if isSimpleType && primType == PrimType.String
=>
new SpecifiedLengthImplicitCharacters(this, body,
this.maxLength.longValue)
-
- case LengthKind.Implicit if isSimpleType && primType ==
PrimType.HexBinary =>
- new SpecifiedLengthImplicit(this, body, this.maxLength.longValue *
bitsMultiplier)
case LengthKind.Implicit
- if isSimpleType && impliedRepresentation == Representation.Binary =>
+ if isSimpleType &&
+ impliedRepresentation == Representation.Binary &&
+ primType != PrimType.HexBinary =>
new SpecifiedLengthImplicit(this, body, implicitBinaryLengthInBits)
Review Comment:
This condition feels like it doesn't exclude enough things. For example,
this matches all binary types except hex binary. But, for example, integers
with length kind implicit use something like `BinaryIntegerKnownLength`
primitive/parser, which doesn't rely on SpecifiedLengthImplicit parser. It
doesn't necessarily hurt, but it's just unnecessary work and might slow things
down, since the BinaryIntegerKnownLengthParser is going to do that work.
I'm wonder if it's just string types that really make use of specified
length stuff?
##########
daffodil-codegen-c/src/main/scala/org/apache/daffodil/codegen/c/DaffodilCCodeGenerator.scala:
##########
@@ -289,7 +289,8 @@ object DaffodilCCodeGenerator
case g: ElementParseAndUnspecifiedLength =>
elementParseAndUnspecifiedLengthGenerateCode(g, cgState)
case g: ElementUnused => noop(g)
- case g: HexBinaryLengthPrefixed =>
hexBinaryLengthPrefixedGenerateCode(g.e, cgState)
+ case g: HexBinaryEndOfBitLimit if g.e.isPrefixed =>
Review Comment:
Hmm, maybe my suggestion was wrong and the primitive still wants to be
something like HexBInaryLengthPrefixed so that the grammar is obvious and
things like code generators/etc can use the obvious grammar names? The parsers
generated don't necessarily have to match the names.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -1387,14 +1381,15 @@ trait ElementBaseGrammarMixin
}
case LengthKind.Implicit if isSimpleType && primType == PrimType.String
=>
new SpecifiedLengthImplicitCharacters(this, body,
this.maxLength.longValue)
-
- case LengthKind.Implicit if isSimpleType && primType ==
PrimType.HexBinary =>
- new SpecifiedLengthImplicit(this, body, this.maxLength.longValue *
bitsMultiplier)
case LengthKind.Implicit
- if isSimpleType && impliedRepresentation == Representation.Binary =>
+ if isSimpleType &&
+ impliedRepresentation == Representation.Binary &&
+ primType != PrimType.HexBinary =>
new SpecifiedLengthImplicit(this, body, implicitBinaryLengthInBits)
- case LengthKind.Implicit if isComplexType =>
+ case LengthKind.Implicit =>
body // for complex types, implicit means "roll up from the bottom"
+ // for simple types, the primitives have custom parsers that handle
implicit length logic
+ // and don't use the limit provided by the SpecifiedLengthImplicit parser
Review Comment:
Can you move this comment above `body`? Single line comments can be on the
same line, but multi-line comments should start before the line.
--
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]
