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/28 14:27:44 UTC
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #482: Update debugger support of found delimiters/fields/diffs
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