[GitHub] [daffodil] mbeckerle commented on a change in pull request #681: Fix some code generation corner cases

[GitBox <gi...@apache.org>]

mbeckerle commented on a change in pull request #681:
URL: https://github.com/apache/daffodil/pull/681#discussion_r754465889



##########
File path: daffodil-runtime2/src/main/scala/org/apache/daffodil/runtime2/generators/CodeGeneratorState.scala
##########
@@ -594,10 +639,10 @@ class CodeGeneratorState {
       s"""// clang-format off
          |#include "generated_code.h"
          |#include <math.h>       // for NAN
-         |#include <stdbool.h>    // for true, false, bool
+         |#include <stdbool.h>    // for false, bool, true
          |#include <stddef.h>     // for NULL, size_t
          |#include <string.h>     // for memset, memcmp
-         |#include "errors.h"     // for Error, PState, UState, ERR_CHOICE_KEY, UNUSED
+         |#include "errors.h"     // for Error, PState, UState, ERR_CHOICE_KEY, Error::(anonymous), UNUSED

Review comment:
       What is "Error::(anonymous)"? I am not familiar with what this is notationally. 
   
   These lines are maintained by some tool I believe. What is that tool trying to tell us with this?

##########
File path: daffodil-runtime2/src/main/scala/org/apache/daffodil/runtime2/generators/CodeGeneratorState.scala
##########
@@ -36,11 +37,32 @@ import scala.collection.mutable
  * Builds up the state of generated code.
  */
 class CodeGeneratorState {
-  private val structs = mutable.Stack[ComplexCGState]()
-  private val prototypes = mutable.ArrayBuffer[String]()
-  private val erds = mutable.ArrayBuffer[String]()
-  private val finalStructs = mutable.ArrayBuffer[String]()
-  private val finalImplementation = mutable.ArrayBuffer[String]()
+  private val elementsAlreadySeen = mutable.Map.empty[String, ElementBase]

Review comment:
       Why is calling the empty method preferable to construction? These are mutable. You need a new instance. Calling empty seems very odd here. 

##########
File path: daffodil-runtime2/src/test/scala/org/apache/daffodil/runtime2/TestCodeGenerator.scala
##########
@@ -168,4 +174,40 @@ class TestCodeGenerator {
     assert(ur.isError, "expected ur.isError to be true")
     assert(ur.getDiagnostics.nonEmpty, "expected ur.getDiagnostics to be non-empty")
   }
+
+  private def updateGeneratedCodeExample(schemaFile: os.Path, rootName: Option[String],
+                                         exampleCodeHeader: os.Path, exampleCodeFile: os.Path): Unit = {
+    // Generate code from the example schema file
+    val pf = Compiler().compileFile(schemaFile.toIO, rootName)
+    assert(!pf.isError, pf.getDiagnostics.map(_.getMessage()).mkString("\n"))
+    val cg = pf.forLanguage("c")
+    val rootNS = QName.refQNameFromExtendedSyntax(rootName.getOrElse("")).toOption
+    val codeDir = cg.generateCode(rootNS, tempDir.toString)
+    assert(!cg.isError, cg.getDiagnostics.map(_.getMessage()).mkString("\n"))
+    val generatedCodeHeader = codeDir/"libruntime"/"generated_code.h"
+    val generatedCodeFile = codeDir/"libruntime"/"generated_code.c"
+
+    // Replace the example generated code files
+    os.copy(generatedCodeHeader, exampleCodeHeader, replaceExisting = true)
+    os.copy(generatedCodeFile, exampleCodeFile, replaceExisting = true)
+  }

Review comment:
       So, if I sbt test, and the output has changed, the tests fail, but a side effect of running those tests, is that the comparison files are updated, so if I sbt test again, it passes?
   
   This seems very problematic to me. 
   
   Make 'sbt test' always idempotent. Same behavior every time. Updating output files to be match new stuff *should* be a manual step. You can provide some automation support for this to make it a one-shot action to update them all, but having 'sbt test' be non-idempotent is asking for trouble. 
   

##########
File path: daffodil-runtime2/src/main/scala/org/apache/daffodil/runtime2/generators/CodeGeneratorState.scala
##########
@@ -157,30 +178,41 @@ class CodeGeneratorState {
    * - we can store the accessed value in an int64_t local variable safely
    */
   private def choiceDispatchField(context: ElementBase): String = {
-    // We want to use SchemaComponent.scPath but it's private so duplicate it here (for now...)
+    // We want to call SchemaComponent.scPath but it's private so duplicate it here for now
     def scPath(sc: SchemaComponent): Seq[SchemaComponent] = sc.optLexicalParent.map { scPath }.getOrElse(Nil) :+ sc
-    val localNames = scPath(context).map {
-      case er: AbstractElementRef => er.refQName.local
-      case e: ElementBase => e.namedQName.local
-      case ed: GlobalElementDecl => ed.namedQName.local
-      case _ => ""
-    }
-    val absoluteSlashPath = localNames.mkString("/")
-    val dispatchSlashPath = context.complexType.modelGroup match {
+
+    // We handle only direct dispatch choices, so ignore other elements
+    context.complexType.modelGroup match {
       case choice: Choice if choice.isDirectDispatch =>
+        // Get parent path against which to perform up paths
+        val parentNames = scPath(context).map {
+          case _: Root => ""
+          case er: AbstractElementRef => er.refQName.local
+          case e: ElementBase => e.namedQName.local
+          case ed: GlobalElementDecl => ed.namedQName.local
+          case _ => ""
+        }
+        val parentPath = parentNames.mkString("/")
+
+        // Convert expression to a relative path (may have up paths)
         val expr = choice.choiceDispatchKeyEv.expr.toBriefXML()
         val before = "'{xs:string("
         val after = ")}'"
         val relativePath = if (expr.startsWith(before) && expr.endsWith(after))
           expr.substring(before.length, expr.length - after.length) else expr
-        val normalizedURI = new URI(absoluteSlashPath + "/" + relativePath).normalize
-        normalizedURI.getPath.substring(1)
-      case _ => ""
+
+        // Remove redundant slashes (//) and up paths (../)
+        val normalizedURI = new URI(parentPath + "/" + relativePath).normalize
+
+        // Strip namespace prefixes since C code uses only local names (for now)
+        val dispatchPath = normalizedURI.getPath.replaceAll("/[^/:]+:", "/")
+
+        // Convert to C struct dot notation without any leading dot
+        val notation = dispatchPath.replace('/', '.').substring(1)
+        notation
+      case _ =>
+        ""

Review comment:
       This line is kind of invisible. Why is returning empty strng ok here? This is the case where the choice is not a direct dispatch choice. I would expect an SDE to be issued here. 
   
   At least add a comment to say why returning "" is ok here. Ie., point out where the error is detected somewhere else. 




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