You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by "olabusayoT (via GitHub)" <gi...@apache.org> on 2024/10/15 23:03:26 UTC
[PR] Remove unnecessary truncation code [daffodil]
olabusayoT opened a new pull request, #1341:
URL: https://github.com/apache/daffodil/pull/1341
- we were truncating and overwriting value, but we didn't actually need to since we ended up truncating again right after. Removed the unnecessary and buggy code
- add tests
DAFFODIL-1592
--
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
Re: [PR] Remove unnecessary truncation code [daffodil]
Posted by "jadams-tresys (via GitHub)" <gi...@apache.org>.
jadams-tresys commented on code in PR #1341:
URL: https://github.com/apache/daffodil/pull/1341#discussion_r1803000438
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/SpecifiedLengthUnparsers.scala:
##########
@@ -87,122 +86,31 @@ final class SpecifiedLengthExplicitImplicitUnparser(
// but we don't know if the length units characters or bits/bytes.
lengthUnits match {
case LengthUnits.Bits | LengthUnits.Bytes =>
- unparseVarWidthCharactersInBits(state)
- case LengthUnits.Characters =>
- unparseVarWidthCharactersInCharacters(state)
+ eUnparser.unparse1(state)
+ case LengthUnits.Characters => {
+ //
+ // variable-width encodings and lengthUnits characters, and lengthKind explicit
+ // is not supported (currently) for complex types
+ //
+ state.schemaDefinitionUnless(
Review Comment:
Just trying to remember, is it still an SDE if it is something that just isn't supported yet?
--
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
Re: [PR] Remove unnecessary truncation code [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT merged PR #1341:
URL: https://github.com/apache/daffodil/pull/1341
--
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
Re: [PR] Remove unnecessary truncation code [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1341:
URL: https://github.com/apache/daffodil/pull/1341#discussion_r1803010139
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/SpecifiedLengthUnparsers.scala:
##########
@@ -87,122 +86,31 @@ final class SpecifiedLengthExplicitImplicitUnparser(
// but we don't know if the length units characters or bits/bytes.
lengthUnits match {
case LengthUnits.Bits | LengthUnits.Bytes =>
- unparseVarWidthCharactersInBits(state)
- case LengthUnits.Characters =>
- unparseVarWidthCharactersInCharacters(state)
+ eUnparser.unparse1(state)
+ case LengthUnits.Characters => {
+ //
+ // variable-width encodings and lengthUnits characters, and lengthKind explicit
+ // is not supported (currently) for complex types
+ //
+ state.schemaDefinitionUnless(
Review Comment:
Good point, I think we usually use `subset`/`subsetError`, which is an SDE but mentions it's a "subset".
--
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
Re: [PR] Remove unnecessary truncation code [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1341:
URL: https://github.com/apache/daffodil/pull/1341#discussion_r1803005632
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/SpecifiedLengthUnparsers.scala:
##########
@@ -87,122 +86,31 @@ final class SpecifiedLengthExplicitImplicitUnparser(
// but we don't know if the length units characters or bits/bytes.
lengthUnits match {
case LengthUnits.Bits | LengthUnits.Bytes =>
- unparseVarWidthCharactersInBits(state)
- case LengthUnits.Characters =>
- unparseVarWidthCharactersInCharacters(state)
+ eUnparser.unparse1(state)
+ case LengthUnits.Characters => {
+ //
+ // variable-width encodings and lengthUnits characters, and lengthKind explicit
+ // is not supported (currently) for complex types
+ //
+ state.schemaDefinitionUnless(
+ erd.isSimpleType,
+ "Variable width character encoding '%s', dfdl:lengthKind '%s' and dfdl:lengthUnits '%s' are not supported for complex types.",
+ getCharset(state).name,
+ lengthKind.toString,
+ lengthUnits.toString
+ )
+
+ Assert.invariant(erd.isSimpleType)
+
+ // truncation is done by StringMaybeTruncateCharactersUnparser
+ eUnparser.unparse1(state, erd)
+ }
Review Comment:
It seems `SpecifiedLengthExplicitImplicitUnparser` doesn't really do anything anymore. I wonder if it can just be greatly simplified? The binary and fixed width text cases call unparseBits which just call `eUnparser.unpase1(...)` (it evaluates target length but I'm not sure if that's needed anymore?). And now the non-fixed width text case also calls `eUnparser.unparse1(...)`, just with an SDE check for complex types.
Feels like all of the real work is now moved out of this unparser and into others. Can this just be simplified to
```scala
if (text representation AND lengthUnits characters AND non fixed width AND isComplex) {
SDE(...)
} else {
// the child unparser inspects target length and handles truncation and padding/fill
// nothing needs to be done in this unparser
eUnparser.unparse1(...)
}
```
And literally everything else about targetLength and unparseBits goes away?
--
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