stevedlawrence commented on code in PR #1334:
URL: https://github.com/apache/daffodil/pull/1334#discussion_r1797205512
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/BinaryNumberTraits.scala:
##########
@@ -103,6 +103,18 @@ trait PrefixedLengthParserMixin {
"Prefixed length result must be non-negative after
dfdl:prefixIncludesPrefixLength adjustment , but was: %d",
adjustedLen
)
+ // do checks on facets expressed on prefixLengthType
+ val optSTRD = plElement.erd.optSimpleTypeRuntimeData
+ if (optSTRD.isDefined) {
+ val strd = optSTRD.get
+ val check = strd.executeCheck(plElement)
+ if (check.isError) {
+ val pe = state.toProcessingError(
+ s"The calculated value of ${prefixedLengthERD.namedQName}
($adjustedLen) failed check due to ${check.errMsg}"
Review Comment:
I wonder if this message should include `parsedLen` instead of `adjustedLen`?
For example, say we have `prefixIncludesPrefixLength="yes"` in the
`PrefixFacetsCheck` test below. In that case the adjusted length would be 4
(since we need to subtract the length of the 1-byte prefix element). The facet
check would still fail since it's checked against the parsedLength in the
infoset (5), but we would output an error like"
> s"The calculated value of prefix2 (4) failed check due to facet
maxInclusive (4)"
Which would be confusing since that looks fine.
That said, maybe the facet checks should be run on the adjusted value and
not the parsed value? In which case we would need to change the infoset value
to the adjusted length prior to calling executeCheck. I think the spec implies
facet checks are run on the parsed value since there's no mention of adjustment.
@mbeckerle, can you clarify?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -192,6 +192,11 @@ trait ElementBaseGrammarMixin
"%s is specified as a dfdl:prefixLengthType, but specifies a
dfdl:trailingSkip other than 0",
prefixLengthType
)
+ schemaDefinitionWhen(
+ detachedElementDecl.statements.nonEmpty,
+ "%s is specified as a dfdl:prefixLengthType, but specifies one or more
statement annotations",
Review Comment:
Can you update the message to include which "statement annotations" it
contains. Users that aren't familiar with the term "statement annotation"
(which sounds kindof generic so is understandable) might be confused what the
problem is. If we instead say it contains "dfdl:assert", for example, that
would be more useful.
--
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]
