You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by GitBox <gi...@apache.org> on 2021/04/21 00:11:42 UTC
[GitHub] [daffodil] mbeckerle opened a new pull request #536: WIP: Added -Y warning check options
mbeckerle opened a new pull request #536:
URL: https://github.com/apache/daffodil/pull/536
These 4 were added.
"-Ywarn-infer-any",
"-Ywarn-inaccessible",
"-Ywarn-nullary-unit",
"-Ywarn-unused-import"
Unwise to merge this until we're at a stable point.
Honestly the warn-nullary-unit change was a bunch of lines of code changed for basically no value.
It doesn't require that the calls to the functions be changed to also have the parentheses.
I.e., it wants
`def init(): Unit = { ... }`
but you can still, without warning call this by just saying `init`. No parentheses.
DAFFODIL-2145
--
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:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on a change in pull request #536: WIP: Added -Y warning check options
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #536:
URL: https://github.com/apache/daffodil/pull/536#discussion_r617754227
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/unparsers/UState.scala
##########
@@ -386,7 +386,7 @@ final class UStateForSuspension(
override def advanceAccessor: InfosetAccessor = die
override def inspect: Boolean = die
override def inspectAccessor: InfosetAccessor = die
- override def fini: Unit = {
+ override def fini(): Unit = {
Review comment:
This finalize method not being called suggests it is not needed. Could be dead code.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DFDLDelimiter.scala
##########
@@ -445,7 +445,7 @@ class CharDelim(val char: Char, ignoreCase: Boolean) extends DelimBase {
}
lazy val typeName = "CharDelim"
- def print = {
+ def print() = {
Review comment:
Dead code - left over from debugging. Remove or comment out fully? Ditto for all other print() methods in this file.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/unparsers/UState.scala
##########
@@ -530,7 +530,7 @@ final class UStateMain private (
override def advanceAccessor: InfosetAccessor = inputter.advanceAccessor
override def inspect: Boolean = inputter.inspect
override def inspectAccessor: InfosetAccessor = inputter.inspectAccessor
- override def fini: Unit = { inputter.fini }
+ override def fini(): Unit = { inputter.fini }
Review comment:
ditto. Dead code?
##########
File path: daffodil-io/src/main/scala/org/apache/daffodil/io/DataOutputStreamImplMixin.scala
##########
@@ -1078,7 +1080,7 @@ trait DataOutputStreamImplMixin extends DataStreamCommonState
setMaybeRelBitLimit0b(savedBitLimit0b, true)
}
- final override def validateFinalStreamState: Unit = {
+ final override def validateFinalStreamState(): Unit = {
Review comment:
This not being called indicates a bug or dead code.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DFDLDelimiter.scala
##########
@@ -412,7 +412,7 @@ class Delimiter {
abstract class DelimBase extends Base {
def typeName: String
- def print: Unit
+ def print(): Unit
Review comment:
Might be left over debug method. Removed?
--
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:
users@infra.apache.org
[GitHub] [daffodil] tuxji commented on a change in pull request #536: WIP: Added -Y warning check options
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #536:
URL: https://github.com/apache/daffodil/pull/536#discussion_r764357331
##########
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
override def advanceOrError = die
override def isInspectArrayEnd = die
-
override def currentInfosetNodeStack = die
- override def currentInfosetNodeMaybe = Maybe(currentInfosetNode)
-
override def arrayIndexStack = die
override def moveOverOneArrayIndexOnly() = die
- override def arrayPos = occursIndex
-
override def groupIndexStack = die
override def moveOverOneGroupIndexOnly() = die
- override def groupPos = 0 // was die, but this is called when copying state during debugging
-
override def childIndexStack = die
override def moveOverOneElementChildOnly() = die
- override def childPos = 0 // was die, but this is called when copying state during debugging.
-
override def pushDelimiters(node: DelimiterStackUnparseNode) = die
override def popDelimiters() = die
+ // $COVERAGE-ON$
+
+ override def groupPos = 0 // was die, but this is called when copying state during debugging
+ override def currentInfosetNodeMaybe = Maybe(currentInfosetNode)
+ override def arrayPos = occursIndex
+ override def childPos = 0 // was die, but this is called when copying state during debugging.
+
Review comment:
I looked at why groupPos and childPos's lines (420, 423) still aren't covered by tests (their original locations were inside the $COVERAGE-OFF$ block). My best guess is that we don't have a test forcing the interactive debugger to copy unparser state. I'll still give you a +1.
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #536: WIP: Added -Y warning check options
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #536:
URL: https://github.com/apache/daffodil/pull/536#discussion_r764178404
##########
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:
Dead code. Base is never called.
--
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
[GitHub] [daffodil] tuxji commented on a change in pull request #536: WIP: Added -Y warning check options
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #536:
URL: https://github.com/apache/daffodil/pull/536#discussion_r764269798
##########
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:
I found a StackOverflow [answer](https://stackoverflow.com/questions/7600910/difference-between-function-with-parentheses-and-without) which said that calling nullary methods without an explicit () is no longer optional in Scala 2.13 or 3.0. BTW, the change wasn't made to annoy people; it was a necessary side effect of another change to improve type inference by allowing eta expansion of Scala functions. What this tells me is that you shouldn't bother changing `initialize` from a def to a lazy val. When we upgrade to 2.13, then scalac will enforce consistency between calls and definitions and we can fix any remaining calls without () at that time.
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #536: WIP: Added -Y warning check options
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #536:
URL: https://github.com/apache/daffodil/pull/536#discussion_r764188938
##########
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:
So this is, believe it or not, an interesting style point.
If I keep initialize as a def, then the warnings will warn that it must have def initialize(): Unit (with the args () thing).
But, it will happily still let me call it with OR without the ().
If I make it a val or lazy val, then calls to it cannot have the (), which enforces consistency between calls and definitions.
What I want is to say, since it is a Unit-returning side-effect function, I want the definition to be def initialize(): Unit, but I also would like it enforced that calls must use the () syntax as well.
But alas, they don't warn on the calls not having the (). IntelliJ suggests adding them, but scalac doesn't warn on them.
So what is your preference. Use def as in def initialize(): Unit, where the () indicate it is a def for side effect, or lazy val which forces consistency?
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #536: WIP: Added -Y warning check options
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #536:
URL: https://github.com/apache/daffodil/pull/536#discussion_r764297845
##########
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:
This is super helpful. So use () when the method has side effects, but no args. Leave out parens if it is side-effect free.
Note that a lazy val that only reads things is still considered side-effect free, even though it has this lazy-val once-only behavior. So if you define a def, intending to possibly override with a lazy val, then no parameters is correct, as a lazy val can't have even an empty argument list.
Also, **_conceptual_** side-effect free applies. Initializing something that happens once as part of setup of some sort is NOT considered a side effect.
A side-effect is something that has to happen *every* time the method is used. E.g, like assigning something each time, or doing an I/O operation each time.
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #536: WIP: Added -Y warning check options
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #536:
URL: https://github.com/apache/daffodil/pull/536#discussion_r764181626
##########
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:
dead code. Will fix.
--
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
[GitHub] [daffodil] tuxji commented on a change in pull request #536: WIP: Added -Y warning check options
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #536:
URL: https://github.com/apache/daffodil/pull/536#discussion_r618411715
##########
File path: daffodil-io/src/main/scala/org/apache/daffodil/io/DataOutputStreamImplMixin.scala
##########
@@ -1078,7 +1080,7 @@ trait DataOutputStreamImplMixin extends DataStreamCommonState
setMaybeRelBitLimit0b(savedBitLimit0b, true)
}
- final override def validateFinalStreamState: Unit = {
+ final override def validateFinalStreamState(): Unit = {
Review comment:
IDEA told me the base DataStreamCommon.validateFinalStreamState() has one caller: line 520 in PState.scala (PState.verifyFinalState). The object calling validateFinalStreamState() is var dataInputStream: InputSourceDataInputStream, a mutable member field of PState.
PState.verifyFinalState() has one caller: line 513 in DataProcessor.scala (DataProcessor.doParse). The object calling verifyFinalState() is state: PState, an input parameter of doParse.
DataProcessor.doParse has one caller: line 436 in DataProcessor.scala (DataProcessor.parse). The object calling doParse is DataProcessor itself and the input parameter it is passing is val state: PState = PState.createInitialPState(..., input, ...), a local object defined inside doParse. The input: InputSourceDataInputStream is an input parameter of DataProcessor.parse.
DataProcessor.parse has 12 callers including Main.scala in daffodil-cli, although I see the call happening in a case of a match expression with the other case calling parseWithSAX. Does Daffodil have two code paths (parsing without SAX and parsing with SAX)? I would think we still test the parsing without SAX code path in CLI and unit tests, especially with all the other 11 calls I see going on, but that's all I can figure out.
--
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:
users@infra.apache.org
[GitHub] [daffodil] tuxji commented on a change in pull request #536: WIP: Added -Y warning check options
Posted by 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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #536: WIP: Added -Y warning check options
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #536:
URL: https://github.com/apache/daffodil/pull/536#discussion_r764191499
##########
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:
Yeah. I think this should be without the (). Clearly scala is not warning about this. It was warning about it at one point.
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #536: WIP: Added -Y warning check options
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #536:
URL: https://github.com/apache/daffodil/pull/536#discussion_r621594066
##########
File path: daffodil-io/src/main/scala/org/apache/daffodil/io/DataOutputStreamImplMixin.scala
##########
@@ -1078,7 +1080,7 @@ trait DataOutputStreamImplMixin extends DataStreamCommonState
setMaybeRelBitLimit0b(savedBitLimit0b, true)
}
- final override def validateFinalStreamState: Unit = {
+ final override def validateFinalStreamState(): Unit = {
Review comment:
TDML runner explicitly runs both regular and sax APIs.
--
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:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #536: WIP: Added -Y warning check options
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #536:
URL: https://github.com/apache/daffodil/pull/536#discussion_r617485375
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/oolag/OOLAG.scala
##########
@@ -308,7 +308,7 @@ object OOLAG extends Logging {
* Unconditionally, evaluate the LV arg in order to insure all checks for this
* object are performed.
*/
- private def requiredEvaluationsAlways(lv: OOLAGValueBase) : Unit = {
+ private def requiredEvaluationsAlways(lv: OOLAGValueBase) (): Unit = {
Review comment:
A few of the changes in this file look incorrect--looks like theres an extra set of parens. Is this a accidental find/replace due to the space before the colon?
##########
File path: daffodil-propgen/src/test/scala/org/apache/daffodil/propGen/TestPropertyGenerator.scala
##########
@@ -65,7 +65,7 @@ class TestPropertyGenerator {
val mx = pg.genAttributeGroup(sch)
assertTrue(mx.contains(""" = convertToBoolean(findProperty("prefixIncludesPrefixLength").value)"""))
assertTrue(mx.contains("""LengthKindMixin"""))
- assertTrue(mx.contains("""def lengthPropertiesAGInit() : Unit = {"""))
+ assertTrue(mx.contains("""def lengthPropertiesAGInit() (): Unit = {"""))
Review comment:
Same issue here
##########
File path: daffodil-propgen/src/main/scala/org/apache/daffodil/propGen/PropertyGenerator.scala
##########
@@ -595,7 +595,7 @@ object Currency {
def generateNonEnumStringInit(pgName: String, propInits: Seq[String]) = {
val initFuncName = initialLowerCase(pgName)
"""
- def """ + initFuncName + """Init() : Unit = {""" +
+ def """ + initFuncName + """Init() (): Unit = {""" +
Review comment:
This looks off too, I think this ends up generated an extra set of parens.
--
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:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on a change in pull request #536: WIP: Added -Y warning check options
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #536:
URL: https://github.com/apache/daffodil/pull/536#discussion_r617510477
##########
File path: daffodil-propgen/src/test/scala/org/apache/daffodil/propGen/TestPropertyGenerator.scala
##########
@@ -65,7 +65,7 @@ class TestPropertyGenerator {
val mx = pg.genAttributeGroup(sch)
assertTrue(mx.contains(""" = convertToBoolean(findProperty("prefixIncludesPrefixLength").value)"""))
assertTrue(mx.contains("""LengthKindMixin"""))
- assertTrue(mx.contains("""def lengthPropertiesAGInit() : Unit = {"""))
+ assertTrue(mx.contains("""def lengthPropertiesAGInit() (): Unit = {"""))
Review comment:
Yup. I'm counting on the CI to find the bugs for me.
--
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:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on a change in pull request #536: WIP: Added -Y warning check options
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #536:
URL: https://github.com/apache/daffodil/pull/536#discussion_r764307123
##########
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:
Actually, based on this. We really can't use "-Ywarn-nullary-unit", because it forces us to use () for def that want to be overridden as lazy val.
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #536: WIP: Added -Y warning check options
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #536:
URL: https://github.com/apache/daffodil/pull/536#discussion_r764178150
##########
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:
Dead code. Base is never called.
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #536: WIP: Added -Y warning check options
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #536:
URL: https://github.com/apache/daffodil/pull/536#discussion_r763511130
##########
File path: daffodil-io/src/main/scala/org/apache/daffodil/io/DataOutputStreamImplMixin.scala
##########
@@ -1078,7 +1080,7 @@ trait DataOutputStreamImplMixin extends DataStreamCommonState
setMaybeRelBitLimit0b(savedBitLimit0b, true)
}
- final override def validateFinalStreamState: Unit = {
+ final override def validateFinalStreamState(): Unit = {
Review comment:
Not dead code.
--
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
[GitHub] [daffodil] jw3 commented on pull request #536: WIP: Added -Y warning check options
Posted by GitBox <gi...@apache.org>.
jw3 commented on pull request #536:
URL: https://github.com/apache/daffodil/pull/536#issuecomment-824079587
Using Scalafix makes some of these operations less painful... but some others more painful.
https://scalacenter.github.io/scalafix/
see #388 #393
--
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:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on a change in pull request #536: WIP: Added -Y warning check options
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #536:
URL: https://github.com/apache/daffodil/pull/536#discussion_r617555341
##########
File path: daffodil-propgen/src/test/scala/org/apache/daffodil/propGen/TestPropertyGenerator.scala
##########
@@ -65,7 +65,7 @@ class TestPropertyGenerator {
val mx = pg.genAttributeGroup(sch)
assertTrue(mx.contains(""" = convertToBoolean(findProperty("prefixIncludesPrefixLength").value)"""))
assertTrue(mx.contains("""LengthKindMixin"""))
- assertTrue(mx.contains("""def lengthPropertiesAGInit() : Unit = {"""))
+ assertTrue(mx.contains("""def lengthPropertiesAGInit() (): Unit = {"""))
Review comment:
Strange. But yes you can
`def foo(): Unit = { .... }`
then invoke with just
`foo` not `foo()`
And scala supplies the implied added '()'. So it seems it can do this even for
`def foo()(): Unit = { .... }`
which invokes with
`foo()`, again supplying the second added '()' implicitly.
Grrrr. Feels like a bug, not a feature.
I'll chase down all ") ()".
I've been thinking about Scala 2.13 and Scala 3.0 eventually, but unless we cross-support them (like we did for 2.11 and 2.12 for a time) so that 2.12 continues to be supported, we're stuck because other important Scala libraries like Apache Spark are still on 2.12.
In other words, I suspect we have to turn on these other warnings that we need for code-quality again without waiting for new scala versions. I.e., in Scala 2.12, and cope with them best we can. E.g., I think the warn on unused and warn on dead-code are both pretty important things for the compiler to do for us.
A wildcard worthy of investigation is whether the Java modules stuff (introduced in Java 9 I believe, and which seems not to be the OGSI stuff all over again) allows one to link together modules with different dependencies, so long as the crossing between them is a a Java API.
--
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:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on a change in pull request #536: WIP: Added -Y warning check options
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #536:
URL: https://github.com/apache/daffodil/pull/536#discussion_r763511064
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/unparsers/UState.scala
##########
@@ -386,7 +386,7 @@ final class UStateForSuspension(
override def advanceAccessor: InfosetAccessor = die
override def inspect: Boolean = die
override def inspectAccessor: InfosetAccessor = die
- override def fini: Unit = {
+ override def fini(): Unit = {
Review comment:
Not dead.
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #536: WIP: Added -Y warning check options
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #536:
URL: https://github.com/apache/daffodil/pull/536#discussion_r764321437
##########
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:
This code is back to being a def with no args.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #536: WIP: Added -Y warning check options
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #536:
URL: https://github.com/apache/daffodil/pull/536#discussion_r617522667
##########
File path: daffodil-propgen/src/test/scala/org/apache/daffodil/propGen/TestPropertyGenerator.scala
##########
@@ -65,7 +65,7 @@ class TestPropertyGenerator {
val mx = pg.genAttributeGroup(sch)
assertTrue(mx.contains(""" = convertToBoolean(findProperty("prefixIncludesPrefixLength").value)"""))
assertTrue(mx.contains("""LengthKindMixin"""))
- assertTrue(mx.contains("""def lengthPropertiesAGInit() : Unit = {"""))
+ assertTrue(mx.contains("""def lengthPropertiesAGInit() (): Unit = {"""))
Review comment:
Looks likethe CI passed, I guess this is technically valid syntax since scala allows multiple parameter lists for currying, and I guess it has no problem with an empty extra parameter list?
--
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:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on a change in pull request #536: WIP: Added -Y warning check options
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #536:
URL: https://github.com/apache/daffodil/pull/536#discussion_r764183727
##########
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:
IntelliJ removed this automatically I assume.
Gen is defined in scalacheck/scalacheck_2.12/scalacheck_2.12-1.15.4-sources.jar!/org/scalacheck/Gen.scala
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #536: WIP: Added -Y warning check options
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #536:
URL: https://github.com/apache/daffodil/pull/536#discussion_r764321639
##########
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:
fixed
--
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
[GitHub] [daffodil] mbeckerle merged pull request #536: WIP: Added -Y warning check options
Posted by GitBox <gi...@apache.org>.
mbeckerle merged pull request #536:
URL: https://github.com/apache/daffodil/pull/536
--
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