[GitHub] [daffodil] tuxji commented on a change in pull request #536: WIP: Added -Y warning check options

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

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



##########
File path: build.sbt
##########
@@ -157,6 +157,25 @@ lazy val commonSettings = Seq(
   crossScalaVersions := Seq("2.12.15"),
   scalacOptions ++= buildScalacOptions(scalaVersion.value),
   Compile / compile / javacOptions ++= buildJavacOptions(),
+  scalacOptions ++= Seq(
+    "-feature",
+    "-deprecation",
+    "-language:experimental.macros",
+    "-unchecked",
+    "-Xfatal-warnings",
+    "-Xxml:-coalescing",
+    "-Xfuture",

Review comment:
       All those options were moved to another place in build.sbt (look for `def buildScalacOptions(scalaVersion: String)` on line 205 in this file).  

##########
File path: daffodil-io/src/main/scala/org/apache/daffodil/io/StringDataInputStreamForUnparse.scala
##########
@@ -84,5 +84,4 @@ final class StringDataInputStreamForUnparse
   override def skip(nBits: Long, finfo: FormatInfo): Boolean = doNotUse
   override def resetBitLimit0b(savedBitLimit0b: MaybeULong): Unit = doNotUse
   // $COVERAGE-ON$
-  override def validateFinalStreamState: Unit = {} // does nothing

Review comment:
       If this removal is valid. also remove the `// $COVERAGE-ON$`.

##########
File path: build.sbt
##########
@@ -157,6 +157,25 @@ lazy val commonSettings = Seq(
   crossScalaVersions := Seq("2.12.15"),
   scalacOptions ++= buildScalacOptions(scalaVersion.value),
   Compile / compile / javacOptions ++= buildJavacOptions(),
+  scalacOptions ++= Seq(
+    "-feature",
+    "-deprecation",
+    "-language:experimental.macros",
+    "-unchecked",
+    "-Xfatal-warnings",
+    "-Xxml:-coalescing",
+    "-Xfuture",
+    "-Ywarn-infer-any",
+    "-Ywarn-inaccessible",
+    "-Ywarn-nullary-unit",
+    "-Ywarn-unused-import"
+  ),
+  // Workaround issue that some options are valid for javac, not javadoc.
+  // These javacOptions are for code compilation only. (Issue sbt/sbt#355)
+  Compile / compile / javacOptions  ++= Seq(
+    "-Werror",
+    "-Xlint:deprecation"

Review comment:
       These javac options were also moved to another place in build.sbt (look for `def buildJavacOptions()` on line 233 in this file).

##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dsom/CompiledExpression1.scala
##########
@@ -223,7 +223,7 @@ class DPathCompileInfo(
     with PreSerialization
     with HasSchemaFileLocation {
 
-  def initialize: Unit = {
+  lazy val initialize: Unit = {
     parents
   }

Review comment:
       Why this change, since I don't see any computation being done?

##########
File path: daffodil-io/src/main/scala/org/apache/daffodil/io/DataOutputStreamImplMixin.scala
##########
@@ -1081,10 +1076,6 @@ trait DataOutputStreamImplMixin extends DataStreamCommonState
     setMaybeRelBitLimit0b(savedBitLimit0b, true)
   }
 
-  final override def validateFinalStreamState: Unit = {
-    // nothing to validate
-  }

Review comment:
       Are you removing this code because it's never called or because the base method does nothing too (in particular, it doesn't throw a ??? exception)?

##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/unparsers/UState.scala
##########
@@ -397,34 +397,31 @@ final class UStateForSuspension(
   override def withUnparserDataInputStream = mainUState.withUnparserDataInputStream
   override def withByteArrayOutputStream = mainUState.withByteArrayOutputStream
 
+  // $COVERAGE-OFF$
   override def advance: Boolean = die
   override def advanceAccessor: InfosetAccessor = die
   override def inspect: Boolean = die
   override def inspectAccessor: InfosetAccessor = die
-  override def fini: Unit = {
-    //do nothing
-  }
+  override def fini(): Unit = die
   override def inspectOrError = die

Review comment:
       Why does `fini()` get parentheses but `inspectOrError` doesn't get parentheses?

##########
File path: daffodil-io/src/main/scala/org/apache/daffodil/io/DataStreamCommon.scala
##########
@@ -98,14 +98,4 @@ trait DataStreamCommon {
    */
   def pastData(nBytesRequested: Int): ByteBuffer
   def futureData(nBytesRequested: Int): ByteBuffer
-
-  /**
-   * Called once after each parse operation to verify final invariants for
-   * the implementation.
-   *
-   * Use to perform checks such as that data structures held in pools are
-   * all returned before end of parse.
-   */
-  def validateFinalStreamState: Unit

Review comment:
       Are you removing this code for a valid reason?

##########
File path: daffodil-lib/src/test/scala/passera/test/UnsignedCheck.scala
##########
@@ -30,14 +30,12 @@ import org.scalacheck._
 // import org.scalacheck.ConsoleReporter
 import org.scalacheck.Prop._
 import passera.unsigned._
-
 import org.junit.Test
 import org.junit.Assert._
 
 import scala.language.implicitConversions
 
 class UnsignedCheck {
-  import Gen._

Review comment:
       Removing an import should be safe as long as the code still compiles, but where is Gen defined?  I ran `rg -uu -w Gen` over my entire fully built checkout and it found only three lines, none of which appears to define Gen:
   
   ```
   daffodil-lib/src/test/scala/passera/test/UnsignedCheck.scala
   40:  import Gen._
   52:  def genUInt: Gen[UInt] = for (n <- arbitrary[Int]) yield UInt(n)
   61:  val nonNegLong = Gen.choose(0L, 0x00000000ffffffffL)
   ```




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