stevedlawrence commented on code in PR #1090:
URL: https://github.com/apache/daffodil/pull/1090#discussion_r1343947069
##########
build.sbt:
##########
@@ -250,14 +250,18 @@ def buildScalacOptions(scalaVersion: String) = {
"-deprecation",
"-language:experimental.macros",
"-unchecked",
- "-Xfatal-warnings",
"-Xxml:-coalescing",
"-Xfuture",
"-Ywarn-infer-any",
"-Ywarn-inaccessible",
// "-Ywarn-nullary-unit", // we cannot use this. It interferes with the
Uniform Access Principle.
// See
https://stackoverflow.com/questions/7600910/difference-between-function-with-parentheses-and-without.
- "-Ywarn-unused-import",
+ "-Ywarn-unused-import"
+ ) ++ (
+ if (!scala.util.Properties.isJavaAtLeast("21"))
+ Seq("-Xfatal-warnings") // Java21 warns about java 8 target being
deprecated
Review Comment:
Instead of removing xfatal warnings for java 21, should we change
`javaVersionSpecificOptions` to something like this:
```scala
val sup = scala.util.Properties
if (sup.isJavaAtLeast("21")) Seq()
else if (sup.isJavaAtLeast("11")) Seq("--release", "8")
else if (sup.isJavaAtLeast("9") Seq("-release", "8")
else Seq()
```
So that on Java 21, you get Java 21 .class files? Or Maybe make it
`--release 9` or `--release 11` or something, so even when building on Java 21
you get some level of backwards compatibility?
##########
build.sbt:
##########
@@ -268,28 +272,30 @@ def buildScalacOptions(scalaVersion: String) = {
case _ => Seq.empty
}
- val javaVersionSpecificOptions =
- if (scala.util.Properties.isJavaAtLeast("9"))
- Seq("-release", "8") // ensure Java backwards compatibility
(DAFFODIL-2579)
- else
- Seq.empty
+ commonOptions ++ scalaVersionSpecificOptions
Review Comment:
This removes the `-release` option from scalacOptions. I'm not sure it
actually matters since it looks like scalac defaults to generating Java 8 class
files regardless of the JVM version. I assume Java 21 will still support
running Java 8 files, just not creating them, so this should be fine?
Or maybe we add a new `val minSupportedJavaVersion` that defaults to 8 but
is 9 or 11 on Java 21, e.g.
```scala
val minSupportedJavaVersion = if (scala.util.Properties.isJavaAtLeast("21")
11 else 8
```
And then we add `Seq("--release", minSupportedJavaVersion)` to both
`scalacOptions` and `javacOptions`? So every class, regardless if it comes from
a .java or .scala file, is the same class file version?
And when we decide to drop Java 8 support, we can just bump this one
variable?
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/xml/XMLUtils.scala:
##########
@@ -850,6 +886,14 @@ Differences were (path, expected, actual):
arrayCounters
}
+ private def getXSIType(a: Elem) = {
+ val res = a.attribute(XSI_NAMESPACE.toString,
"type").map(_.head.text).orElse {
+ val a2 = a.attributes.asAttrMap.get("xsi:type")
Review Comment:
This `a2` is new. Is this because `xmlns:xsi` isn't define in some infosets?
Should we just define them so they are valid XML?
##########
build.sbt:
##########
@@ -268,28 +272,30 @@ def buildScalacOptions(scalaVersion: String) = {
case _ => Seq.empty
}
- val javaVersionSpecificOptions =
- if (scala.util.Properties.isJavaAtLeast("9"))
- Seq("-release", "8") // ensure Java backwards compatibility
(DAFFODIL-2579)
- else
- Seq.empty
+ commonOptions ++ scalaVersionSpecificOptions
+}
- commonOptions ++ scalaVersionSpecificOptions ++ javaVersionSpecificOptions
+val javaVersionSpecificOptions = {
+ val sup = scala.util.Properties
+ if (sup.isJavaAtLeast("11"))
+ Seq("--release", "8")
+ else if (sup.isJavaAtLeast("9"))
+ // this -release option has been removed in recent Java versions
+ Seq("-release", "8") // ensure Java backwards compatibility (DAFFODIL-2579)
+ else // java 8
+ Seq.empty
}
// Workaround issue that some options are valid for javac, not javadoc.
// These javacOptions are for code compilation only. (Issue sbt/sbt#355)
def buildJavacOptions() = {
val commonOptions = Seq(
- "-Werror",
- "-Xlint:deprecation",
- )
-
- val javaVersionSpecificOptions =
- if (scala.util.Properties.isJavaAtLeast("9"))
- Seq("--release", "8") // ensure Java backwards compatibility
(DAFFODIL-2579)
+ "-Xlint:deprecation") ++ (
+ if (!scala.util.Properties.isJavaAtLeast("21"))
+ Seq("-Werror") // Java21 warns about java 8 target being deprecated
else
- Seq.empty
+ Nil
+ )
Review Comment:
Is this change still needed now that ``--release`` is not used when building
with Java 21?
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/xml/XMLUtils.scala:
##########
@@ -1113,6 +1189,29 @@ Differences were (path, expected, actual):
val b = DFDLDateTimeConversion.fromXMLString(dataB)
a == b
}
+ case Some("xs:double") => {
+ val a = strToDouble(dataA)
+ val b = strToDouble(dataB)
Review Comment:
Mentioned above, but this could be `NodeInfo.Double.fromXMLString(...)`.
Same with xs:float below.
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/util/Misc.scala:
##########
@@ -103,38 +102,99 @@ object Misc {
// more time is wasted by people forgetting that the initial "/" is needed
// to get classpath relative behavior... Let's make sure there is a
leading "/"
val resPath = if (resourcePath.startsWith("/")) resourcePath else "/" +
resourcePath
- val res = this.getClass().getResource(resPath)
- if (res == null) {
- (None, resPath)
- } else (Some(res.toURI), resPath)
+ val res = Option(this.getClass().getResource(resPath))
+ (res.map.(_.toURI), resPath)
Review Comment:
Typo `.` here.
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/xml/XMLUtils.scala:
##########
@@ -70,6 +70,34 @@ object XMLUtils {
val NegativeInfinityString = "-INF"
val NaNString = "NaN"
+ /**
+ * Converts a string to a float, including handling our INF, -INF, and NaN
notations.
+ * @param s
+ * @return
+ */
+ def strToFloat(s: String): Float = {
+ s match {
+ case PositiveInfinityString => Float.PositiveInfinity
+ case NegativeInfinityString => Float.NegativeInfinity
+ case NaNString => Float.NaN
+ case _ => s.toFloat
+ }
+ }
+
+ /**
+ * Converts a string to a double, including handling our INF, -INF, and NaN
notations.
+ * @param s
+ * @return
+ */
+ def strToDouble(s: String): Double = {
+ s match {
+ case PositiveInfinityString => Double.PositiveInfinity
+ case NegativeInfinityString => Double.NegativeInfinity
+ case NaNString => Double.NaN
+ case _ => s.toDouble
+ }
+ }
+
Review Comment:
The `NodeInfo.Float.fromXMLString` and `NodeInfo.Double.fromXMLString`
functions do something similar with a little extra error checking.
--
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]
