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 2021/01/27 15:22:42 UTC
[GitHub] [incubator-daffodil] stevedlawrence opened a new pull request #482: Update debugger support of found delimiters/fields/diffs
stevedlawrence opened a new pull request #482:
URL: https://github.com/apache/incubator-daffodil/pull/482
- Refactor how diff's are calculated so that all the logic is moved into
the individual command objects that care about that piece of
information. Now, any info command that is "diffable" must implement
the InfoDiffable trait and the 'diff' function. The "info diff"
command now automatically finds all commands that implement this trait
and will call this diff function. This simplifies the info diff
command and makes it so it doesn't need to know anything about other
info commands or anything about the state.
- Instead of "info foundDelimiter" showing both the found delimiter and
the found field, it now only shows the found delimiter. And add a new
"info foundField" command to show the found field that "info
foundDelimiter" used to show. This makes the code less complex and
gives the user more control over what information they want to see.
These new commands are also made diffable so they now show up in "info
diff". When diffs are found, this command outputs something like this:
```
diff:
foundDelimiter: none -> ,
foundField: none -> smith
```
The above shows that a comma delimiter was found and that the field
proceeding that delimiter was "smith".
DAFFODIL-758
> Note this doesn't fully resolve DAFFODIL-758, this is just an incremental step in improving trace output.
----------------------------------------------------------------
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] bsloane1650 commented on pull request #482: Update debugger support of found delimiters/fields/diffs
Posted by GitBox <gi...@apache.org>.
bsloane1650 commented on pull request #482:
URL: https://github.com/apache/incubator-daffodil/pull/482#issuecomment-770094352
New changes look good.
----------------------------------------------------------------
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 pull request #482: Update debugger support of found delimiters/fields/diffs
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #482:
URL: https://github.com/apache/incubator-daffodil/pull/482#issuecomment-769326164
I've added a new commit that adds much more refactoring, including some of the suggestions above. This is almost an RFC level of changes. The message in the new commit describes what changed. I haven't yet added tests to improve coverge, curious what others think of these changes before doing that.
----------------------------------------------------------------
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 #482: Update debugger support of found delimiters/fields/diffs
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #482:
URL: https://github.com/apache/incubator-daffodil/pull/482#discussion_r565463159
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/debugger/InteractiveDebugger.scala
##########
@@ -1638,6 +1716,15 @@ class InteractiveDebugger(runner: InteractiveDebuggerRunner, eCompilers: Express
}
DebugState.Pause
}
+
+ def diff(prestate: StateForDebugger, state: ParseOrUnparseState): Boolean = {
+ if (prestate.arrayPos != state.arrayPos) {
+ debugPrintln("%s: %d -> %d".format(name, prestate.arrayPos, state.arrayPos), " ")
Review comment:
I see now that this change set does not include any CLI tests that drive this new info-diff system, and that's why codecov is complaining. You should add at least one test that just does info diff on a contrived example that exercises all of these diffs.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/debugger/InteractiveDebugger.scala
##########
@@ -1528,37 +1542,91 @@ class InteractiveDebugger(runner: InteractiveDebuggerRunner, eCompilers: Express
}
}
- object InfoFoundDelimiter extends DebugCommand with DebugCommandValidateZeroArgs {
+ object InfoFoundDelimiter extends DebugCommand with DebugCommandValidateZeroArgs with InfoDiffable {
val name = "foundDelimiter"
override lazy val short = "fd"
val desc = "display the current found delimiter"
val longDesc = desc
+
+ private def getDelimOpt(s: StateForDebugger): Option[String] = {
+ if (s.delimitedParseResult.isDefined) {
+ val pr = s.delimitedParseResult.get
+ val delim = Misc.remapStringToVisibleGlyphs(pr.matchedDelimiterValue.get)
+ Some(delim)
+ } else {
+ None
+ }
+ }
+
def act(args: Seq[String], prestate: StateForDebugger, state: ParseOrUnparseState, processor: Processor): DebugState.Type = {
- state match {
- case pstate: PState => {
- if (pstate.delimitedParseResult.isDefined) {
- val pr = pstate.delimitedParseResult.get
- debugPrintln("%s:".format(name))
- debugPrintln("foundField: %s".format(Misc.remapStringToVisibleGlyphs(pr.field.get)), " ")
- debugPrintln("foundDelimiter: %s".format(Misc.remapStringToVisibleGlyphs(pr.matchedDelimiterValue.get)), " ")
- } else {
- debugPrintln("%s: nothing found".format(name))
- }
- }
- case ustate: UState => {
- // TODO
+ val delimOpt = getDelimOpt(state)
+ val str = delimOpt.getOrElse("nothing found")
+ debugPrintln("%s: %s".format(name, str))
Review comment:
Codecov is also complaining about this. Seems like this may be spurious? You showed examples of this. Tests of this would have to be under the daffodil-cli/src/test directory.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/debugger/InteractiveDebugger.scala
##########
@@ -1320,21 +1337,40 @@ class InteractiveDebugger(runner: InteractiveDebuggerRunner, eCompilers: Express
}
DebugState.Pause
}
+
+ def diff(prestate: StateForDebugger, state: ParseOrUnparseState): Boolean = {
+ if (prestate.bitLimit0b != state.bitLimit0b) {
+ debugPrintln("%s: %d -> %d".format(name, prestate.bitLimit0b, state.bitLimit0b), " ")
Review comment:
codecov says this is uncovered. Probably should have a test that outputs ALL diffs.
----------------------------------------------------------------
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 pull request #482: Update debugger support of found delimiters/fields/diffs
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #482:
URL: https://github.com/apache/incubator-daffodil/pull/482#issuecomment-772704520
I've added a couple tests to make sure all the diff code paths are covered. I'm not sure if codecov is going to update this PR, but the results can be seen here:
https://app.codecov.io/gh/apache/incubator-daffodil/compare/482/diff
There's still probably room for improvement in debugger coverage, but this at least covers all the new/existing diff logic.
----------------------------------------------------------------
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] bsloane1650 commented on a change in pull request #482: Update debugger support of found delimiters/fields/diffs
Posted by GitBox <gi...@apache.org>.
bsloane1650 commented on a change in pull request #482:
URL: https://github.com/apache/incubator-daffodil/pull/482#discussion_r565429533
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/debugger/InteractiveDebugger.scala
##########
@@ -1528,37 +1542,91 @@ class InteractiveDebugger(runner: InteractiveDebuggerRunner, eCompilers: Express
}
}
- object InfoFoundDelimiter extends DebugCommand with DebugCommandValidateZeroArgs {
+ object InfoFoundDelimiter extends DebugCommand with DebugCommandValidateZeroArgs with InfoDiffable {
val name = "foundDelimiter"
override lazy val short = "fd"
val desc = "display the current found delimiter"
val longDesc = desc
+
+ private def getDelimOpt(s: StateForDebugger): Option[String] = {
Review comment:
Since we are in runtime1, we want to use Maybe.
This is just the debugger, so Option is probably fine if you actually need the extra functionality.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/debugger/InteractiveDebugger.scala
##########
@@ -1528,37 +1542,91 @@ class InteractiveDebugger(runner: InteractiveDebuggerRunner, eCompilers: Express
}
}
- object InfoFoundDelimiter extends DebugCommand with DebugCommandValidateZeroArgs {
+ object InfoFoundDelimiter extends DebugCommand with DebugCommandValidateZeroArgs with InfoDiffable {
val name = "foundDelimiter"
override lazy val short = "fd"
val desc = "display the current found delimiter"
val longDesc = desc
+
+ private def getDelimOpt(s: StateForDebugger): Option[String] = {
+ if (s.delimitedParseResult.isDefined) {
+ val pr = s.delimitedParseResult.get
+ val delim = Misc.remapStringToVisibleGlyphs(pr.matchedDelimiterValue.get)
+ Some(delim)
+ } else {
+ None
+ }
+ }
+
def act(args: Seq[String], prestate: StateForDebugger, state: ParseOrUnparseState, processor: Processor): DebugState.Type = {
- state match {
- case pstate: PState => {
- if (pstate.delimitedParseResult.isDefined) {
- val pr = pstate.delimitedParseResult.get
- debugPrintln("%s:".format(name))
- debugPrintln("foundField: %s".format(Misc.remapStringToVisibleGlyphs(pr.field.get)), " ")
- debugPrintln("foundDelimiter: %s".format(Misc.remapStringToVisibleGlyphs(pr.matchedDelimiterValue.get)), " ")
- } else {
- debugPrintln("%s: nothing found".format(name))
- }
- }
- case ustate: UState => {
- // TODO
+ val delimOpt = getDelimOpt(state)
+ val str = delimOpt.getOrElse("nothing found")
+ debugPrintln("%s: %s".format(name, str))
+ DebugState.Pause
+ }
+
+ def diff(prestate: StateForDebugger, state: ParseOrUnparseState): Boolean = {
+ val delimOptPre = getDelimOpt(prestate)
+ val delimOptPost = getDelimOpt(state)
+
+ (delimOptPre, delimOptPost) match {
+ case (None, None) => false
+ case (Some(pre), Some(post)) if pre == post => false
Review comment:
These cases are the same: case (a, b) if a == b => false
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/debugger/InteractiveDebugger.scala
##########
@@ -1567,6 +1635,15 @@ class InteractiveDebugger(runner: InteractiveDebuggerRunner, eCompilers: Express
}
DebugState.Pause
}
+
+ def diff(prestate: StateForDebugger, state: ParseOrUnparseState): Boolean = {
+ if (prestate.groupPos != state.groupPos) {
+ debugPrintln("%s: %d -> %d".format(name, prestate.groupPos, state.groupPos), " ")
Review comment:
We are doing this for bassically every diff.
Would it make more sense for the interface to instead be:
```
getValue(state: State): Option[Any]
```
Then let the trait itself implement the diff() function?
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/debugger/InteractiveDebugger.scala
##########
@@ -1528,37 +1542,91 @@ class InteractiveDebugger(runner: InteractiveDebuggerRunner, eCompilers: Express
}
}
- object InfoFoundDelimiter extends DebugCommand with DebugCommandValidateZeroArgs {
+ object InfoFoundDelimiter extends DebugCommand with DebugCommandValidateZeroArgs with InfoDiffable {
val name = "foundDelimiter"
override lazy val short = "fd"
val desc = "display the current found delimiter"
val longDesc = desc
+
+ private def getDelimOpt(s: StateForDebugger): Option[String] = {
+ if (s.delimitedParseResult.isDefined) {
+ val pr = s.delimitedParseResult.get
+ val delim = Misc.remapStringToVisibleGlyphs(pr.matchedDelimiterValue.get)
+ Some(delim)
+ } else {
+ None
+ }
+ }
+
def act(args: Seq[String], prestate: StateForDebugger, state: ParseOrUnparseState, processor: Processor): DebugState.Type = {
- state match {
- case pstate: PState => {
- if (pstate.delimitedParseResult.isDefined) {
- val pr = pstate.delimitedParseResult.get
- debugPrintln("%s:".format(name))
- debugPrintln("foundField: %s".format(Misc.remapStringToVisibleGlyphs(pr.field.get)), " ")
- debugPrintln("foundDelimiter: %s".format(Misc.remapStringToVisibleGlyphs(pr.matchedDelimiterValue.get)), " ")
- } else {
- debugPrintln("%s: nothing found".format(name))
- }
- }
- case ustate: UState => {
- // TODO
+ val delimOpt = getDelimOpt(state)
+ val str = delimOpt.getOrElse("nothing found")
+ debugPrintln("%s: %s".format(name, str))
+ DebugState.Pause
+ }
+
+ def diff(prestate: StateForDebugger, state: ParseOrUnparseState): Boolean = {
+ val delimOptPre = getDelimOpt(prestate)
+ val delimOptPost = getDelimOpt(state)
+
+ (delimOptPre, delimOptPost) match {
+ case (None, None) => false
+ case (Some(pre), Some(post)) if pre == post => false
+ case (_, _) => {
+ val strPre = delimOptPre.getOrElse("none")
+ val strPost = delimOptPost.getOrElse("none")
+ debugPrintln("%s: %s -> %s".format(name, strPre, strPost), " ")
+ true
}
- case _ => Assert.impossibleCase()
}
+ }
+ }
+
+ object InfoFoundField extends DebugCommand with DebugCommandValidateZeroArgs with InfoDiffable {
+ val name = "foundField"
+ override lazy val short = "ff"
+ val desc = "display the current found field when delimiter scanning"
+ val longDesc = desc
+
+ private def getFieldOpt(s: StateForDebugger): Option[String] = {
+ if (s.delimitedParseResult.isDefined) {
+ val pr = s.delimitedParseResult.get
+ val field = Misc.remapStringToVisibleGlyphs(pr.field.get)
+ Some(field)
+ } else {
+ None
+ }
+ }
+
+ def act(args: Seq[String], prestate: StateForDebugger, state: ParseOrUnparseState, processor: Processor): DebugState.Type = {
+ val fieldOpt = getFieldOpt(state)
+ val str = fieldOpt.getOrElse("nothing found")
+ debugPrintln("%s: %s".format(name, str))
DebugState.Pause
}
+
+ def diff(prestate: StateForDebugger, state: ParseOrUnparseState): Boolean = {
+ val fieldOptPre = getFieldOpt(prestate)
+ val fieldOptPost = getFieldOpt(state)
+ (fieldOptPre, fieldOptPost) match {
+ case (None, None) => false
+ case (Some(pre), Some(post)) if pre == post => false
+ case (_, _) => {
+ val strPre = fieldOptPre.getOrElse("none")
+ val strPost = fieldOptPost.getOrElse("none")
Review comment:
See InfoFoundDelimiter
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/debugger/InteractiveDebugger.scala
##########
@@ -1459,47 +1505,15 @@ class InteractiveDebugger(runner: InteractiveDebuggerRunner, eCompilers: Express
override lazy val short = "diff"
val desc = "display the differences from the previous state"
val longDesc = desc
+
+ private lazy val infoDiffables = Info.subcommands.collect { case diffable: InfoDiffable => diffable }
+
def act(args: Seq[String], prestate: StateForDebugger, state: ParseOrUnparseState, processor: Processor): DebugState.Type = {
debugPrintln("%s:".format(name))
- var diff = false
- (prestate, state) match {
- case (prestate: StateForDebugger, state: ParseOrUnparseState) => {
- if (prestate.bytePos != state.bytePos) { debugPrintln("position (bytes): %d -> %d".format(prestate.bytePos, state.bytePos), " "); diff = true }
- if (prestate.bitLimit0b != state.bitLimit0b) { debugPrintln("bitLimit: %d -> %d".format(prestate.bitLimit0b, state.bitLimit0b), " "); diff = true }
- if (prestate.arrayPos != state.arrayPos) { debugPrintln("occursIndex: %d -> %d".format(prestate.arrayPos, state.arrayPos), " "); diff = true }
- if (prestate.groupPos != state.groupPos) { debugPrintln("groupIndex: %d -> %d".format(prestate.groupPos, state.groupPos), " "); diff = true }
- if (prestate.childPos != state.childPos) { debugPrintln("childIndex: %d -> %d".format(prestate.childPos, state.childPos), " "); diff = true }
- prestate.variableMap.qnames.foreach { qname =>
- val pre_instance = prestate.variableMap.find(qname).get
- val pre_value = pre_instance.value
- val pre_state = pre_instance.state
-
- val cur_instance = state.variableMap.find(qname).get
- val cur_value = cur_instance.value
- val cur_state = cur_instance.state
-
- if (pre_value != cur_value || pre_state != cur_state) {
- debugPrintln("variable: %s: %s -> %s".format(
- qname,
- InfoVariables.variableInstanceToDebugString(pre_instance),
- InfoVariables.variableInstanceToDebugString(cur_instance),
- ), " ")
- diff = true
- }
- }
- }
- case _ => // ok
+ val differencesExist = infoDiffables.foldLeft(false) { case (foundDiff, ic) =>
+ ic.diff(prestate, state) || foundDiff
Review comment:
At first glance, this looks like it could be: infoDiffables.exists(_.diff(prestate, state)).
This doesn't work, because we are relying on the side effect of diff calling debugPrint.
It is very unclear here that this expression has any side effect.
The more FP style approach would be to have diff return Maybe[String], and let the caller collect and print the messages.
Practically; perhaps we could rename diff -> printDiff to make it clear that diff is effectfull.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/debugger/InteractiveDebugger.scala
##########
@@ -1528,37 +1542,91 @@ class InteractiveDebugger(runner: InteractiveDebuggerRunner, eCompilers: Express
}
}
- object InfoFoundDelimiter extends DebugCommand with DebugCommandValidateZeroArgs {
+ object InfoFoundDelimiter extends DebugCommand with DebugCommandValidateZeroArgs with InfoDiffable {
val name = "foundDelimiter"
override lazy val short = "fd"
val desc = "display the current found delimiter"
val longDesc = desc
+
+ private def getDelimOpt(s: StateForDebugger): Option[String] = {
+ if (s.delimitedParseResult.isDefined) {
+ val pr = s.delimitedParseResult.get
+ val delim = Misc.remapStringToVisibleGlyphs(pr.matchedDelimiterValue.get)
+ Some(delim)
+ } else {
+ None
+ }
+ }
+
def act(args: Seq[String], prestate: StateForDebugger, state: ParseOrUnparseState, processor: Processor): DebugState.Type = {
- state match {
- case pstate: PState => {
- if (pstate.delimitedParseResult.isDefined) {
- val pr = pstate.delimitedParseResult.get
- debugPrintln("%s:".format(name))
- debugPrintln("foundField: %s".format(Misc.remapStringToVisibleGlyphs(pr.field.get)), " ")
- debugPrintln("foundDelimiter: %s".format(Misc.remapStringToVisibleGlyphs(pr.matchedDelimiterValue.get)), " ")
- } else {
- debugPrintln("%s: nothing found".format(name))
- }
- }
- case ustate: UState => {
- // TODO
+ val delimOpt = getDelimOpt(state)
+ val str = delimOpt.getOrElse("nothing found")
+ debugPrintln("%s: %s".format(name, str))
+ DebugState.Pause
+ }
+
+ def diff(prestate: StateForDebugger, state: ParseOrUnparseState): Boolean = {
+ val delimOptPre = getDelimOpt(prestate)
+ val delimOptPost = getDelimOpt(state)
+
+ (delimOptPre, delimOptPost) match {
+ case (None, None) => false
+ case (Some(pre), Some(post)) if pre == post => false
+ case (_, _) => {
+ val strPre = delimOptPre.getOrElse("none")
+ val strPost = delimOptPost.getOrElse("none")
Review comment:
Could just do: case (Some(strPre), Some(strPost)) instead of deconstructing again.
----------------------------------------------------------------
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 #482: Update debugger support of found delimiters/fields/diffs
Posted by GitBox <gi...@apache.org>.
stevedlawrence merged pull request #482:
URL: https://github.com/apache/incubator-daffodil/pull/482
----------------------------------------------------------------
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 #482: Update debugger support of found delimiters/fields/diffs
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #482:
URL: https://github.com/apache/incubator-daffodil/pull/482#discussion_r565456084
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/debugger/InteractiveDebugger.scala
##########
@@ -1459,47 +1505,15 @@ class InteractiveDebugger(runner: InteractiveDebuggerRunner, eCompilers: Express
override lazy val short = "diff"
val desc = "display the differences from the previous state"
val longDesc = desc
+
+ private lazy val infoDiffables = Info.subcommands.collect { case diffable: InfoDiffable => diffable }
+
def act(args: Seq[String], prestate: StateForDebugger, state: ParseOrUnparseState, processor: Processor): DebugState.Type = {
debugPrintln("%s:".format(name))
- var diff = false
- (prestate, state) match {
- case (prestate: StateForDebugger, state: ParseOrUnparseState) => {
- if (prestate.bytePos != state.bytePos) { debugPrintln("position (bytes): %d -> %d".format(prestate.bytePos, state.bytePos), " "); diff = true }
- if (prestate.bitLimit0b != state.bitLimit0b) { debugPrintln("bitLimit: %d -> %d".format(prestate.bitLimit0b, state.bitLimit0b), " "); diff = true }
- if (prestate.arrayPos != state.arrayPos) { debugPrintln("occursIndex: %d -> %d".format(prestate.arrayPos, state.arrayPos), " "); diff = true }
- if (prestate.groupPos != state.groupPos) { debugPrintln("groupIndex: %d -> %d".format(prestate.groupPos, state.groupPos), " "); diff = true }
- if (prestate.childPos != state.childPos) { debugPrintln("childIndex: %d -> %d".format(prestate.childPos, state.childPos), " "); diff = true }
- prestate.variableMap.qnames.foreach { qname =>
- val pre_instance = prestate.variableMap.find(qname).get
- val pre_value = pre_instance.value
- val pre_state = pre_instance.state
-
- val cur_instance = state.variableMap.find(qname).get
- val cur_value = cur_instance.value
- val cur_state = cur_instance.state
-
- if (pre_value != cur_value || pre_state != cur_state) {
- debugPrintln("variable: %s: %s -> %s".format(
- qname,
- InfoVariables.variableInstanceToDebugString(pre_instance),
- InfoVariables.variableInstanceToDebugString(cur_instance),
- ), " ")
- diff = true
- }
- }
- }
- case _ => // ok
+ val differencesExist = infoDiffables.foldLeft(false) { case (foundDiff, ic) =>
+ ic.diff(prestate, state) || foundDiff
Review comment:
Yeah, agreed this isn't very functional. This whole debugger isn't very functional though. Basically both act and diff have sideeffecting printlning. I think I'd rather just go with printDiff since this we already scatter debugPrintln all over the place. Espeically since the caller
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/debugger/InteractiveDebugger.scala
##########
@@ -1528,37 +1542,91 @@ class InteractiveDebugger(runner: InteractiveDebuggerRunner, eCompilers: Express
}
}
- object InfoFoundDelimiter extends DebugCommand with DebugCommandValidateZeroArgs {
+ object InfoFoundDelimiter extends DebugCommand with DebugCommandValidateZeroArgs with InfoDiffable {
val name = "foundDelimiter"
override lazy val short = "fd"
val desc = "display the current found delimiter"
val longDesc = desc
+
+ private def getDelimOpt(s: StateForDebugger): Option[String] = {
+ if (s.delimitedParseResult.isDefined) {
+ val pr = s.delimitedParseResult.get
+ val delim = Misc.remapStringToVisibleGlyphs(pr.matchedDelimiterValue.get)
+ Some(delim)
+ } else {
+ None
+ }
+ }
+
def act(args: Seq[String], prestate: StateForDebugger, state: ParseOrUnparseState, processor: Processor): DebugState.Type = {
- state match {
- case pstate: PState => {
- if (pstate.delimitedParseResult.isDefined) {
- val pr = pstate.delimitedParseResult.get
- debugPrintln("%s:".format(name))
- debugPrintln("foundField: %s".format(Misc.remapStringToVisibleGlyphs(pr.field.get)), " ")
- debugPrintln("foundDelimiter: %s".format(Misc.remapStringToVisibleGlyphs(pr.matchedDelimiterValue.get)), " ")
- } else {
- debugPrintln("%s: nothing found".format(name))
- }
- }
- case ustate: UState => {
- // TODO
+ val delimOpt = getDelimOpt(state)
+ val str = delimOpt.getOrElse("nothing found")
+ debugPrintln("%s: %s".format(name, str))
+ DebugState.Pause
+ }
+
+ def diff(prestate: StateForDebugger, state: ParseOrUnparseState): Boolean = {
+ val delimOptPre = getDelimOpt(prestate)
+ val delimOptPost = getDelimOpt(state)
+
+ (delimOptPre, delimOptPost) match {
+ case (None, None) => false
+ case (Some(pre), Some(post)) if pre == post => false
+ case (_, _) => {
+ val strPre = delimOptPre.getOrElse("none")
+ val strPost = delimOptPost.getOrElse("none")
Review comment:
Then I need separate cases for ``case (Some(strPre), None)`` and `` case(None, Some(strPost))``. I guess this could just be ``case _ => ...`` so there's not checking if the type is a tuple. Though, I guess this all simplifies to not even needing match case anymore. Just becomes something like
```scala
if (delimOptPre == delimOptPost) {
false
} else {
val strPost = delimOptPre.getOrElse("none")
....
}
```
Maybe that's better than the match/case that isn't really necessary?
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/debugger/InteractiveDebugger.scala
##########
@@ -1528,37 +1542,91 @@ class InteractiveDebugger(runner: InteractiveDebuggerRunner, eCompilers: Express
}
}
- object InfoFoundDelimiter extends DebugCommand with DebugCommandValidateZeroArgs {
+ object InfoFoundDelimiter extends DebugCommand with DebugCommandValidateZeroArgs with InfoDiffable {
val name = "foundDelimiter"
override lazy val short = "fd"
val desc = "display the current found delimiter"
val longDesc = desc
+
+ private def getDelimOpt(s: StateForDebugger): Option[String] = {
Review comment:
Yeah, I figured that since we're in the debugger any performance guarantees go out the window, and the Option makes the code a bit cleaner since you can't do a match/case with a Maybe.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/debugger/InteractiveDebugger.scala
##########
@@ -1567,6 +1635,15 @@ class InteractiveDebugger(runner: InteractiveDebuggerRunner, eCompilers: Express
}
DebugState.Pause
}
+
+ def diff(prestate: StateForDebugger, state: ParseOrUnparseState): Boolean = {
+ if (prestate.groupPos != state.groupPos) {
+ debugPrintln("%s: %d -> %d".format(name, prestate.groupPos, state.groupPos), " ")
Review comment:
I'm expecting there to be more complicated diff coming downt the pipe. We already have variables which is quite different, and I'm starting work on the delimiter stack which is also going to be very different I think.
Maybe a new trait like ``InfoDiffableEquals`` or something that have a default implement of ``def diff`` and a ``getValue(state: StateForDebugger): scala.Equals`` type of thing would be cleaner for these common cases?
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/debugger/InteractiveDebugger.scala
##########
@@ -1528,37 +1542,91 @@ class InteractiveDebugger(runner: InteractiveDebuggerRunner, eCompilers: Express
}
}
- object InfoFoundDelimiter extends DebugCommand with DebugCommandValidateZeroArgs {
+ object InfoFoundDelimiter extends DebugCommand with DebugCommandValidateZeroArgs with InfoDiffable {
val name = "foundDelimiter"
override lazy val short = "fd"
val desc = "display the current found delimiter"
val longDesc = desc
+
+ private def getDelimOpt(s: StateForDebugger): Option[String] = {
+ if (s.delimitedParseResult.isDefined) {
+ val pr = s.delimitedParseResult.get
+ val delim = Misc.remapStringToVisibleGlyphs(pr.matchedDelimiterValue.get)
+ Some(delim)
+ } else {
+ None
+ }
+ }
+
def act(args: Seq[String], prestate: StateForDebugger, state: ParseOrUnparseState, processor: Processor): DebugState.Type = {
- state match {
- case pstate: PState => {
- if (pstate.delimitedParseResult.isDefined) {
- val pr = pstate.delimitedParseResult.get
- debugPrintln("%s:".format(name))
- debugPrintln("foundField: %s".format(Misc.remapStringToVisibleGlyphs(pr.field.get)), " ")
- debugPrintln("foundDelimiter: %s".format(Misc.remapStringToVisibleGlyphs(pr.matchedDelimiterValue.get)), " ")
- } else {
- debugPrintln("%s: nothing found".format(name))
- }
- }
- case ustate: UState => {
- // TODO
+ val delimOpt = getDelimOpt(state)
+ val str = delimOpt.getOrElse("nothing found")
+ debugPrintln("%s: %s".format(name, str))
+ DebugState.Pause
+ }
+
+ def diff(prestate: StateForDebugger, state: ParseOrUnparseState): Boolean = {
+ val delimOptPre = getDelimOpt(prestate)
+ val delimOptPost = getDelimOpt(state)
+
+ (delimOptPre, delimOptPost) match {
+ case (None, None) => false
+ case (Some(pre), Some(post)) if pre == post => false
Review comment:
Yep, will fix.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/debugger/InteractiveDebugger.scala
##########
@@ -1638,6 +1716,15 @@ class InteractiveDebugger(runner: InteractiveDebuggerRunner, eCompilers: Express
}
DebugState.Pause
}
+
+ def diff(prestate: StateForDebugger, state: ParseOrUnparseState): Boolean = {
+ if (prestate.arrayPos != state.arrayPos) {
+ debugPrintln("%s: %d -> %d".format(name, prestate.arrayPos, state.arrayPos), " ")
Review comment:
Yep, surprised there weren't any tests. I did to manual tests, but I'll make sure to add some real tests that make sure this all works as expected.
----------------------------------------------------------------
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 pull request #482: Update debugger support of found delimiters/fields/diffs
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on pull request #482:
URL: https://github.com/apache/incubator-daffodil/pull/482#issuecomment-769378744
I like the concept of eliminating the before-capture, and using the prior after-state as the incoming before state. Code does look cleaned up a bit.
I think it will be nearly equivalent when used. Instead of showing diffs for only one element you'd have to stop before that element, then single step then info diff to see only the deltas for that one step. Seems fine.
I think the real proof of this will be how well it works out for unparsing.
btw I think when debugging, it's highly likely we want to retry all suspensions at each stop (repeating until no more of them successfully evaluate). At least that should be one mode which is to evaluate suspensions as soon as possible. Perhaps another mode would evaluate them "normally", but I fear this will just be viewed as non-deterministic by users. They won't have any idea why things are being evaluated at various points.
I'm just waiting for some tests to one-plus this.
----------------------------------------------------------------
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