stevedlawrence commented on a change in pull request #466:
URL: https://github.com/apache/incubator-daffodil/pull/466#discussion_r540287487
##########
File path:
daffodil-runtime2/src/main/scala/org/apache/daffodil/runtime2/generators/BinaryIntegerKnownLengthCodeGenerator.scala
##########
@@ -36,50 +38,47 @@ trait BinaryIntegerKnownLengthCodeGenerator {
val bo = e.byteOrderEv.constValue
bo
}
- // For the time being this is a very limited back end.
- // So there are numerous restrictions to enforce.
- e.schemaDefinitionUnless(lengthInBits <= 64, "Length must be 64 bits or
less, but was: %s", lengthInBits)
- if (lengthInBits == 64 && !g.signed)
- e.SDE("Integers of 64 bits length must be signed.")
// This insures we can use regular java.io library calls.
if (e.alignmentValueInBits.intValue() % 8 != 0)
e.SDE("Only alignment to 8-bit (1 byte) boundaries is supported.")
// The restrictions below are ones we want to eventually lift.
- if (lengthInBits != 32)
- e.SDE("Lengths other than 32 bits are not supported.")
-
if (byteOrder ne ByteOrder.BigEndian)
e.SDE("Only dfdl:byteOrder 'bigEndian' is supported.")
if (e.bitOrder ne BitOrder.MostSignificantBitFirst)
e.SDE("Only dfdl:bitOrder 'mostSignificantBitFirst' is supported.")
- if (!g.signed)
- e.SDE("Only signed integers are supported.")
-
- val initStatement = s" instance->$fieldName = 0xCDCDCDCD;"
+ // Start generating code snippets
+ val initialValue = lengthInBits match {
+ case 8 => "0xCD"
+ case 16 => "0xCDCD"
+ case 32 => "0xCDCDCDCD"
+ case 64 => "0xCDCDCDCDCDCDCDCD"
+ case _ => e.SDE("Lengths other than 8, 16, 32, or 64 bits are not
supported.")
Review comment:
What is this 0xCD magic number?
##########
File path: project/Rat.scala
##########
@@ -114,7 +114,8 @@ object Rat {
file("daffodil-sapi/src/test/resources/test/sapi/myData16.dat"),
file("daffodil-sapi/src/test/resources/test/sapi/myData19.dat"),
file("daffodil-sapi/src/test/resources/test/sapi/myDataBroken.dat"),
-
file("daffodil-runtime2/src/test/resources/org/apache/daffodil/runtime2/parse_int32"),
+
file("daffodil-runtime2/src/test/resources/org/apache/daffodil/runtime2/ex_int32_parse"),
+
file("daffodil-runtime2/src/test/resources/org/apache/daffodil/runtime2/orion_command_parse"),
Review comment:
Can you add .bin, .dat or something just so it makes it more clear that
these are binary files? Makes it more obvious the reason why we need to exclude
them from rat.
Also, can these runtime-2 excludes be sorted so they are before
daffodil-sapi?
##########
File path:
daffodil-runtime2/src/main/scala/org/apache/daffodil/runtime2/Runtime2DataProcessor.scala
##########
@@ -108,6 +108,8 @@ class Runtime2DataProcessor(executableFile: os.Path)
extends DFDL.DataProcessorB
val parseResult = new ParseResult(outfile, Failure(parseError))
parseResult.addDiagnostic(parseError)
parseResult
+ } finally {
+ os.remove.all(tempDir)
Review comment:
I just realized this comes from some new dependencies that I missed were
added in a previous commit. Looks like the new deps are:
"com.lihaoyi" %% "os-lib" % "0.7.1", // for generating C source files
"dev.dirs" % "directories" % "21" // for caching compiled C files
Are there any others i missed?
Looks like the first is MIT, which is [Category
A](https://www.apache.org/legal/resolved.html#category-a) and is fine as code
or dependency, but we need to update the appropriate license files. Since this
dependency is distributed in binary form with the CLI, we only need to update
daffodil-cli/bin.LICENSE to include the license information.
Looks like the second is MPL, [Category
B](https://www.apache.org/legal/resolved.html#category-b), which is allowed and
needs a LICENSE update like the other one, but we also need a "prominant label"
that makes it very clear. For this reason, I tend to try to avoid these where
possible.
Some other questions, are these new deps used in runtime2? I'm wondering if
either we have good alternatives already in Daffodil Utils, or if these deps
provide functionality/better code readabliilty that we should be using
througout?
I would prefer that we don't have one runtime that uses a very different
style or utilities just because it makes it more difficult for someone faimilar
with one runtime to contribute to another runtime.
For example, I mentioend a utlity function in another PR that I think would
be useful in Daffodil that handles all this tempdir stuff for us, including
cleanup, that I think would be useful throughout Daffodil. I'm wondering that
if we only use some small subset of these dependencies that there might be
benefit in Daffodil specific implementations. I haven't looked deeply into what
they do or how we use them.
----------------------------------------------------------------
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:
[email protected]
