[GitHub] [daffodil] stevedlawrence commented on a diff in pull request #974: Add checks for bit lengths

["stevedlawrence (via GitHub)" <gi...@apache.org>]

stevedlawrence commented on code in PR #974:
URL: https://github.com/apache/daffodil/pull/974#discussion_r1123144789


##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/ElementBase.scala:
##########
@@ -750,21 +751,46 @@ trait ElementBase
 
   // FIXME: bless this method. Deprecate and remove other less reliable things.
   final lazy val maybeFixedLengthInBits: MaybeULong = {
-    if (isRepresented && repElement.isFixedLength) {
-      val bitsMultiplier = repElement.lengthUnits match {
-        case LengthUnits.Bits => 1
-        case LengthUnits.Bytes => 8
-        case LengthUnits.Characters =>
-          if (knownEncodingIsFixedWidth) knownEncodingWidthInBits else -1
-      }
-      if (bitsMultiplier > 0) {
-        MaybeULong(repElement.fixedLengthValue * bitsMultiplier)
+    val result = {
+      if (isRepresented && repElement.isFixedLength) {
+        val bitsMultiplier = repElement.lengthUnits match {
+          case LengthUnits.Bits => 1
+          case LengthUnits.Bytes => 8
+          case LengthUnits.Characters =>
+            if (knownEncodingIsFixedWidth) knownEncodingWidthInBits else -1
+        }
+        if (bitsMultiplier > 0) {
+          MaybeULong(repElement.fixedLengthValue * bitsMultiplier)
+        } else {
+          MaybeULong.Nope
+        }
       } else {
         MaybeULong.Nope
       }
-    } else {
-      MaybeULong.Nope
     }
+
+    // Validate that the number of bits does not exceed the maximum number of
+    // bits allowed for the type
+    if (
+      result.isDefined && repElement.isSimpleType && representation == Representation.Binary
+    ) {
+      primType match {
+        case primNumeric: NodeInfo.PrimType.PrimNumeric =>
+          if (primNumeric.width.isDefined) {
+            val nBits = result.get
+            val width = primNumeric.width.get
+            if (nBits > width) {
+              SDE(
+                "Number of bits %d out of range, must be between 1 and %d bits.",
+                nBits,
+                width,
+              )

Review Comment:
   Might be useful to add "binary" and the type in here to make it clear the the type doesn't match the bits.
   ```suggestion
                 SDE(
                   "Number of bits %d out of range for binary %s, must be between 1 and %d bits.",
                   nBits,
                   primNumeric.globalQName,
                   width,
                 )
   ```



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