[GitHub] [daffodil] stevedlawrence commented on a diff in pull request #931: Fix issue with groups containing choices: WIP

["stevedlawrence (via GitHub)" <gi...@apache.org>]

stevedlawrence commented on code in PR #931:
URL: https://github.com/apache/daffodil/pull/931#discussion_r1112116827


##########
daffodil-test/src/test/scala/org/apache/daffodil/section15/choice_groups/TestChoice.scala:
##########
@@ -161,7 +161,16 @@ class TestChoice {
   @Test def test_direct_dispatch_17(): Unit = { runnerCH.runOneTest("direct_dispatch_17") }
 
   // DAFFODIL-2562
-  // @Test def test_dispatch_group_choice(): Unit = { runnerCH.runOneTest("dispatch_group_choice") }
+  @Test def test_dispatch_group_choice(): Unit = {
+    runnerCH.runOneTest("dispatch_group_choice")
+  }
+
+  @Test def test_dispatch_group_choice_2(): Unit = {
+    runnerCH.runOneTest("dispatch_group_choice_2")
+  }
+  @Test def test_dispatch_group_choice_3(): Unit = {
+    runnerCH.runOneTest("dispatch_group_choice_3")
+  }

Review Comment:
   Have you run against the DFDL regression suite? The logic seems reasonable, but it also feels like the kind of thing that might have subtle breaking changes?



##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/infoset/PartialNextElementResolver.scala:
##########
@@ -75,6 +77,9 @@ trait NextElementResolver { self: InfosetInputter =>
         // or it will issue an UnparseError if there are required elements in the
         // set of possible elements.
         //
+        Logger.log.debug(s"current trd: $trd")
+        Logger.log.debug(s"resolver: $resolver")
+        Logger.log.debug(s"name: $name\n")

Review Comment:
   I'd suggest that we move these to a single log.debug call with `\n`'s if you want them on a separate line. Each call to log.debug is going to call `isDebugEnabled`, which should be pretty fast, but is dependent on the logger implementation so might have some overhead that would be worth avoiding, especially since this is at runtime.
   
   Agreed with Mike that these all need some kind of prefix to make it obvious what they are about.



-- 
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