mbeckerle commented on code in PR #1058:
URL: https://github.com/apache/daffodil/pull/1058#discussion_r1270771366
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -203,7 +203,7 @@ abstract class Expression extends OOLAGHostImpl() with
BasicComponent {
*/
lazy val targetType: NodeInfo.Kind = {
// this will only be called for expressions that have a parent
- Assert.usage(parentOpt.isDefined)
+ Assert.invariant(parentOpt.isDefined)
Review Comment:
This one is fair to make an invariant check. It's not checking an arg
anyway.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/compiler/Compiler.scala:
##########
@@ -138,7 +138,7 @@ final class ProcessorFactory private (
override def isError: Boolean = sset.isError
def withDistinguishedRootNode(name: String, namespace: String):
ProcessorFactory = {
- Assert.usage(name ne null)
+ Assert.invariant(name ne null)
Review Comment:
This should be a usage error.
An invariant is a check on some property/characteristic that is supposed to
be true. Usually about deeper aspects of things than just whether an arg is
null or not.
A usage check is about the caller making a mistake when calling something.
In neither case however, is this EVER supposed to become an issue for a DFDL
user. Failure of Assert.usage or Assert.invariant are both Daffodil bugs we
need to fix.
Both should throw an unhandled RuntimeException which is only caught by the
CLI in order to issue a "please report this bug" message to a CLI user.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -789,7 +789,7 @@ case class RootPathExpression(relPath:
Option[RelativePathExpression])
}
override def targetTypeForSubexpression(subexp: Expression) = {
- Assert.usage(relPath.isDefined)
+ Assert.invariant(relPath.isDefined)
Review Comment:
This one is also ok as an invariant check as relpath is not an argument.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/compiler/Compiler.scala:
##########
@@ -185,7 +185,7 @@ class Compiler private (
new Compiler(validateDFDLSchemas, tunables, checkAllTopLevel, optRootName,
optRootNamespace)
def withDistinguishedRootNode(name: String, namespace: String): Compiler = {
- Assert.usage(name ne null)
+ Assert.invariant(name ne null)
Review Comment:
Same here, and throughout. Usage errors are for arg-checking. Invariant
checking is for making explicit things the code is assuming are true.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/primitives/SpecifiedLength.scala:
##########
@@ -153,7 +153,7 @@ class SpecifiedLengthImplicit(e: ElementBase, eGram: =>
Gram, nBits: Long)
class SpecifiedLengthPrefixed(e: ElementBase, eGram: => Gram, bitsMultiplier:
Int)
extends SpecifiedLengthCombinatorBase(e, eGram) {
- Assert.usage(bitsMultiplier > 0)
+ Assert.invariant(bitsMultiplier > 0)
Review Comment:
Usage is preferred for arg checks.
##########
daffodil-io/src/main/scala/org/apache/daffodil/io/DataInputStreamImplMixin.scala:
##########
@@ -32,7 +32,7 @@ trait DataInputStreamImplMixin
}
final override def isAligned(bitAlignment1b: Int): Boolean = {
- Assert.usage(bitAlignment1b >= 1)
+ Assert.invariant(bitAlignment1b >= 1)
Review Comment:
Usage is preferred for arg checks.
##########
daffodil-io/src/main/scala/org/apache/daffodil/io/DataOutputStreamImplMixin.scala:
##########
@@ -956,8 +956,8 @@ trait DataOutputStreamImplMixin
bitLengthFrom1To64: Int,
finfo: FormatInfo,
): Boolean = {
- Assert.usage(bitLengthFrom1To64 >= 1 && bitLengthFrom1To64 <= 64)
- Assert.usage(isWritable)
+ Assert.invariant(bitLengthFrom1To64 >= 1 && bitLengthFrom1To64 <= 64)
Review Comment:
Arg checks should be usage.
##########
daffodil-io/src/main/scala/org/apache/daffodil/io/DataOutputStreamImplMixin.scala:
##########
@@ -1150,7 +1150,7 @@ trait DataOutputStreamImplMixin
// pastData from it as part of what it displays.
areDebugging,
)
- Assert.usage(nBytesRequested >= 0)
+ Assert.invariant(nBytesRequested >= 0)
Review Comment:
usage
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/primitives/SpecifiedLength.scala:
##########
@@ -120,7 +120,7 @@ class SpecifiedLengthExplicit(e: ElementBase, eGram: =>
Gram, bitsMultiplier: In
extends SpecifiedLengthCombinatorBase(e, eGram)
with SpecifiedLengthExplicitImplicitUnparserMixin {
- Assert.usage(bitsMultiplier > 0)
+ Assert.invariant(bitsMultiplier > 0)
Review Comment:
Usage is preferred.
##########
daffodil-io/src/main/scala/org/apache/daffodil/io/LimitingJavaIinputStreams.scala:
##########
@@ -99,8 +99,8 @@ object BoundaryMarkLimitingStream {
targetChunkSize: Int = 32 * 1024,
) = {
- Assert.usage(targetChunkSize >= 1)
- Assert.usage(boundaryMark.length >= 1)
+ Assert.invariant(targetChunkSize >= 1)
Review Comment:
both usage
##########
daffodil-io/src/main/scala/org/apache/daffodil/io/DirectOrBufferedDataOutputStream.scala:
##########
@@ -296,15 +296,15 @@ class DirectOrBufferedDataOutputStream private[io] (
}
override def setJavaOutputStream(newOutputStream: java.io.OutputStream):
Unit = {
- Assert.usage(newOutputStream ne null)
+ Assert.invariant(newOutputStream ne null)
Review Comment:
usage
##########
daffodil-io/src/main/scala/org/apache/daffodil/io/DataOutputStreamImplMixin.scala:
##########
@@ -1164,15 +1164,15 @@ trait DataOutputStreamImplMixin
}
override final def resetMaybeRelBitLimit0b(savedBitLimit0b: MaybeULong):
Unit = {
- Assert.usage(isWritable)
+ Assert.invariant(isWritable)
setMaybeRelBitLimit0b(savedBitLimit0b, true)
}
final override def isAligned(bitAlignment1b: Int): Boolean = {
- Assert.usage(bitAlignment1b >= 1)
+ Assert.invariant(bitAlignment1b >= 1)
Review Comment:
usage
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/util/BitOrder.scala:
##########
@@ -44,7 +44,7 @@ object Bits {
def asUnsignedByte(b: Long): Int = (b & 0xff).toInt
def asSignedByte(i: Long): Byte = {
- Assert.usage(i >= 0)
+ Assert.invariant(i >= 0)
Review Comment:
usage
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/exceptions/Assert.scala:
##########
@@ -128,7 +128,7 @@ object Assert extends Assert {
//
def usageError(message: String = "Usage error."): Nothing = {
- abort(message)
+ toss(new IllegalStateException(message))
Review Comment:
I don't think of a usage error as an illegal state exception. If you want to
use a precise Java exception class for this I think there is an
IllegalArgumentException.
I *do* think of an invariant failure as an Illegal state exception.
But we have quite a bit of code depending on the fact that we always throw
only one of our own exceptions so that they can be handled uniformly (or
explicitly NOT handled when somebody wants to be sure they are not being
captured.)
So I think the right thing is to use a Java IllegalArgumentException as the
cause of a Daffodil-specific exception like Abort.
Let's improve the scaladoc on usage-related assert methods. They're about
checking args.
They're also primarily intended for *us* daffodil developers using our own
internal methods.
##########
daffodil-io/src/main/scala/org/apache/daffodil/io/DataOutputStreamImplMixin.scala:
##########
@@ -333,12 +333,12 @@ trait DataOutputStreamImplMixin
def fragmentLastByteLimit = fragmentLastByteLimit_
def setFragmentLastByte(newFragmentByte: Int, nBitsInUse: Int): Unit = {
- Assert.usage(nBitsInUse >= 0 && nBitsInUse <= 7)
- Assert.usage(
+ Assert.invariant(nBitsInUse >= 0 && nBitsInUse <= 7)
Review Comment:
Should be usage
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/util/BitOrder.scala:
##########
@@ -86,7 +86,7 @@ object Bits {
}
def reverseBitsWithinBytes(bb: ByteBuffer): Unit = {
- Assert.usage(bb.position() == 0)
+ Assert.invariant(bb.position() == 0)
Review Comment:
usage
##########
daffodil-runtime1-layers/src/main/scala/org/apache/daffodil/layers/runtime1/LineFoldedTransformer.scala:
##########
@@ -395,9 +395,9 @@ class LineFoldedOutputStream(mode: LineFoldMode, jos:
OutputStream) extends Outp
override def write(bInt: Int): Unit = {
val b = bInt.toChar
- Assert.usage(!closed)
+ Assert.invariant(!closed)
Assert.invariant(line.length <= lineLength)
- Assert.usage(b >= 0)
+ Assert.invariant(b >= 0)
Review Comment:
This bInt check is a usage check.
##########
daffodil-macro-lib/src/main/scala/org/apache/daffodil/lib/exceptions/AssertMacros.scala:
##########
@@ -28,7 +28,7 @@ object AssertMacros {
q"""
if (!($testAbortsIfFalse)) {
- Assert.abort2("Usage error: " + $message, $testAsString)
+ throw new IllegalStateException("Usage error: " + $message)
Review Comment:
If you want this to use a standard java exception it should throw an Abort
with IllegalArgumentException as the cause of the exception. Otherwise I fear
this is going to break quite a lot of stuff.
Changing the class being thrown is a big change. You need to scrutinize
every catch in the code base to see if it has to change correspondingly.
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/StringLengthUnparsers.scala:
##########
@@ -72,7 +72,7 @@ sealed abstract class
StringSpecifiedLengthUnparserTruncateBase(
erd: ElementRuntimeData,
) extends StringSpecifiedLengthUnparserBase(erd) {
- Assert.usage(stringTruncationType ne TextTruncationType.None)
+ Assert.invariant(stringTruncationType ne TextTruncationType.None)
Review Comment:
usage
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/infoset/InfosetInputter.scala:
##########
@@ -657,7 +657,7 @@ object InfosetAccessor {
final class Info private (val v: AnyRef) extends AnyVal with Serializable {
def node = element
def element = {
- Assert.usage(v ne null)
+ Assert.invariant(v ne null)
Review Comment:
usage
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/AnnotatedSchemaComponent.scala:
##########
@@ -81,7 +81,7 @@ trait ResolvesLocalProperties extends FindPropertyMixin {
self: AnnotatedSchemaC
* Does lookup of only local properties
*/
protected override def lookupProperty(pname: String): PropertyLookupResult =
{
- Assert.usage(
+ Assert.invariant(
Review Comment:
This is a usage error.
From here on I'm only going to comment on the ones I think are incorrectly
chnaged to invariant. Many of the changes are fine/correct. Some are not.
##########
daffodil-io/src/main/scala/org/apache/daffodil/io/LimitingJavaIinputStreams.scala:
##########
@@ -162,8 +162,8 @@ class RegexLimitingStream(
targetChunkSize: Int = 32 * 1024,
) extends InputStream {
- Assert.usage(targetChunkSize >= 1)
- Assert.usage(maximumLengthDelimiterExample.length >= 1)
+ Assert.invariant(targetChunkSize >= 1)
Review Comment:
both usage
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/util/BitOrder.scala:
##########
@@ -109,7 +109,7 @@ object Bits {
}
def reverseBytes(bb: ByteBuffer): Unit = {
- Assert.usage(bb.position() == 0)
+ Assert.invariant(bb.position() == 0)
Review Comment:
I'm not going to try to flag all these any more.
If it is checking an arg, that's what Assert.usage is for.
Perhaps we should have a synonym Assert.argCheck(...) ?
##########
daffodil-lib/src/test/scala/org/apache/daffodil/lib/macros/TestAssertMacros.scala:
##########
@@ -41,7 +41,7 @@ class TestAssertMacros {
}
@Test def testUsage1Arg(): Unit = {
- val e = intercept[Abort] {
+ val e = intercept[IllegalStateException] {
Review Comment:
Not correct. Usage is not illegal state.
--
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]
