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 2020/12/14 17:59:25 UTC
[GitHub] [incubator-daffodil] stevedlawrence opened a new pull request #468: Fix abort when viewing prefixed length elements in the debugger
stevedlawrence opened a new pull request #468:
URL: https://github.com/apache/incubator-daffodil/pull/468
The InfosetWalker, which is used for displaying the infoset, expects
elements to have a container element. When creating a detached element
for prefixed elements, we did so without a DIDocument. This causes
assertions to fail or null pointers to be hit. To ensure the detached
element used for prefixed parsing/element is appropriate the
InfosetWalker, create a "detached" DIDocument and the new detached
DIElement to that.
DAFFODIL-2348
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence merged pull request #468: Fix abort when viewing prefixed length elements in the debugger
Posted by GitBox <gi...@apache.org>.
stevedlawrence merged pull request #468:
URL: https://github.com/apache/incubator-daffodil/pull/468
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #468: Fix abort when viewing prefixed length elements in the debugger
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #468:
URL: https://github.com/apache/incubator-daffodil/pull/468#discussion_r543384387
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/SpecifiedLengthUnparsers.scala
##########
@@ -385,8 +388,11 @@ class SpecifiedLengthPrefixedUnparser(
override lazy val childProcessors = Vector(prefixedLengthUnparser, eUnparser)
override def unparse(state: UState): Unit = {
- // create a "detached" element that the prefix length will be used to unparse
- val plElem = Infoset.newElement(prefixedLengthERD).asInstanceOf[DISimple]
+ // Create a "detached" DIDocument with a single child element that the
+ // prefix length will be parsed to. This creates a completely new
+ // infoset and parses to that, so care is taken to ensure this infoset
+ // is only used for the prefix length parsing and is removed afterwards
+ val plElem = Infoset.newDetachedElement(state, prefixedLengthERD).asInstanceOf[DISimple]
Review comment:
I would wait for the next pass of profiling. A DFDL schema that makes extensive use of prefixed lengths (such as for every string) would be interesting to have here, but in my experience that is most common in mainframe data (from PL/1 actually. Cobol data is more commonly fixed length), which we don't (yet) see much of.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #468: Fix abort when viewing prefixed length elements in the debugger
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #468:
URL: https://github.com/apache/incubator-daffodil/pull/468#discussion_r543348390
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/SpecifiedLengthUnparsers.scala
##########
@@ -385,8 +388,11 @@ class SpecifiedLengthPrefixedUnparser(
override lazy val childProcessors = Vector(prefixedLengthUnparser, eUnparser)
override def unparse(state: UState): Unit = {
- // create a "detached" element that the prefix length will be used to unparse
- val plElem = Infoset.newElement(prefixedLengthERD).asInstanceOf[DISimple]
+ // Create a "detached" DIDocument with a single child element that the
+ // prefix length will be parsed to. This creates a completely new
+ // infoset and parses to that, so care is taken to ensure this infoset
+ // is only used for the prefix length parsing and is removed afterwards
+ val plElem = Infoset.newDetachedElement(state, prefixedLengthERD).asInstanceOf[DISimple]
Review comment:
Perhaps an alternative would be to get the current DIDocument and add this detached element as a child of that? That avoids the additional allocation, but then cleanup becomes a bit trickier? I guess it shouldn't be hard to remove that new child? There might also be considerations with the InfosetWalker, it might have some assumptions that the DIDocument only has a single node. Probably surmountable, but does add some complexity.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #468: Fix abort when viewing prefixed length elements in the debugger
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #468:
URL: https://github.com/apache/incubator-daffodil/pull/468#discussion_r542602538
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/SpecifiedLengthUnparsers.scala
##########
@@ -385,8 +388,11 @@ class SpecifiedLengthPrefixedUnparser(
override lazy val childProcessors = Vector(prefixedLengthUnparser, eUnparser)
override def unparse(state: UState): Unit = {
- // create a "detached" element that the prefix length will be used to unparse
- val plElem = Infoset.newElement(prefixedLengthERD).asInstanceOf[DISimple]
+ // Create a "detached" DIDocument with a single child element that the
+ // prefix length will be parsed to. This creates a completely new
+ // infoset and parses to that, so care is taken to ensure this infoset
+ // is only used for the prefix length parsing and is removed afterwards
+ val plElem = Infoset.newDetachedElement(state, prefixedLengthERD).asInstanceOf[DISimple]
Review comment:
I'm a bit concerned about the overhead of this added node, but we should deal with that via profiling. There are many many overheads in Daffodil, no reason to think this one will be the most important.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org