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

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

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



##########
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:
       We already have roundtrip parse/unparse tests for a number of schemas that should have sufficient coverage to let us know if something broke in the generated code.  I want to eliminate unnecessary steps to reduce friction for code contributors making changes to the C code generator.    I would rather have the tests update the expected files automatically instead of forcing code contributors to update the expected files manually.  Once the expected files are updated, code contributors will see any changes in `git diff` output and reviewers will see the changes in a pull request if the code contributor follows our code contributor workflow documentation (e.g., always run the tests first).  If a contributor changes some generator code but doesn't run the tests, the worst case is that those changes will appear in someone else's unrelated pull request, which still will alert us that the changes happened (they won't sneak past or avoid our review for too long).
   

##########
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]
+  private val structs = mutable.Stack.empty[ComplexCGState]
+  private val prototypes = mutable.ArrayBuffer.empty[String]
+  private val erds = mutable.ArrayBuffer.empty[String]
+  private val finalStructs = mutable.ArrayBuffer.empty[String]
+  private val finalImplementation = mutable.ArrayBuffer.empty[String]
+
+  // Returns true if the element has not been seen before (checking if
+  // a map already contains the element, otherwise adding it to the map)
+  def elementNotSeenYet(context: ElementBase): Boolean = {
+    val key = context.namedQName.toString
+    val alreadySeen = elementsAlreadySeen.contains(key)
+    if (!alreadySeen) elementsAlreadySeen += (key -> context)
+    !alreadySeen
+  }

Review comment:
       Yes, this code is intended to handle references to global elements (which have unique qnames) from multiple places in a schema (e.g., a common `Header` element which is interpolated into several `....Message` elements with `<xs:element ref="tns:Header"/>`).  Before this change, we were generating redundant C code (duplicate `struct Header` definitions, duplicate ERDs, etc.).  The schema I'm writing is not for public use so I don't want to put it into an example.




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