tuxji commented on a change in pull request #681:
URL: https://github.com/apache/daffodil/pull/681#discussion_r754556876
##########
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:
The definition of mutable.Map.empty is:
```scala
def empty[K, V]: Map[K, V] = new HashMap[K, V]
```
The other mutable.\<C\>.empty functions also return new instances. There is
no semantic (only syntactic) difference between empty calls (parentheses are
optional), apply calls (parentheses are mandatory), and constructors calls
(`new` is mandatory). I checked and found all three types of mutable
container calls used in Daffodil source code. Nevertheless, I'll switch back
to apply since that idiom is more common and some lines at the end of this same
file use apply as well.
##########
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:
Okay, then I want to remove this test and make "sbt compile" update the
example code files instead. We should automatically generate (a.k.a. update)
the example code files whenever we build Daffodil, just like the build
automatically generates those other files and keeps them up to date after any
changes to the source code:
```scala
lazy val genManaged = taskKey[Unit]("Generate managed sources and resources")
lazy val genProps = taskKey[Seq[File]]("Generate properties scala source")
lazy val genSchemas = taskKey[Seq[File]]("Generated DFDL schemas")
```
All I want is to have pull requests include any changes to the example code
files automatically. I don't want any extra manual step. We have parse and
unparse tests which compile and run the new generated files. These tests will
warn us if any code generator backend changes break the generated files, so
there is no reason why we need to compare the new generated files to the old
files and fail the build even if only one byte differs. It just would be nice
(and helpful for reviewers) to show how code generator backend changes affect
the example code files.
Steve (or Mike), what's the easiest way to make "sbt compile" call
`updateGeneratedCodeExample` without needing to stage the daffodil CLI?
##########
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:
The tool is include-what-you-use, iwyu for short. It's telling us that
we're dereferencing an anonymous union inside the Error struct, which has no
union name but does have a member name:
```c
typedef struct Error
{
uint8_t code;
union
{
int c; // for %c
int64_t d64; // for %d64
const char *s; // for %s
} arg;
} Error;
```
##########
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:
Method addBeforeSwitchStatements in this same file calls
choiceDispatchField on every complexType (group) element and generates some
additional C code only if choiceDispatchField returns a non-blank string:
```scala
val dispatchField = choiceDispatchField(context)
if (dispatchField.nonEmpty) {
```
I've moved the empty string to the same line as the case and added a comment
explaining why returning "" is OK here.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
