kaitai_struct_compiler
kaitai_struct_compiler copied to clipboard
kaitai-io
Document DataTypes and fix some non-exhaustive warnings
Document DataType classes and fix 2 related non-exhaustive warnings.
The following warnings were fixed (from this run):
[warn] /home/runner/work/kaitai_struct_compiler/kaitai_struct_compiler/compiler/shared/src/main/scala/io/kaitai/struct/precompile/CalculateSeqSizes.scala:106:5: match may not be exhaustive.
[warn] It would fail on the following inputs: ArrayTypeInStream(_), BitsType(_, _), BitsType1(_), BytesType(), CalcArrayType(_), CalcBooleanType, CalcIntType, EnumType(_, _), FloatType(), KaitaiStreamType, OwnedKaitaiStreamType, StructType()
[warn] dataType match {
[warn] ^
[warn] /home/runner/work/kaitai_struct_compiler/kaitai_struct_compiler/compiler/shared/src/main/scala/io/kaitai/struct/precompile/CalculateSeqSizes.scala:106:5: match may not be exhaustive.
[warn] It would fail on the following inputs: ArrayTypeInStream(_), BitsType(_, _), BitsType1(_), CalcArrayType(_), CalcBooleanType, CalcIntType, EnumType(_, _), FloatType(), KaitaiStreamType, OwnedKaitaiStreamType, StructType()
[warn] dataType match {
[warn] ^
The following error
[info] - expr_sizeof_value_dynamic3 *** FAILED ***
[info] scala.MatchError: CalcArrayType(Int1Type(false)) (of class io.kaitai.struct.datatype.DataType$CalcArrayType)
[info] at io.kaitai.struct.precompile.CalculateSeqSizes$.dataTypeByteSize(CalculateSeqSizes.scala:106)
[info] at io.kaitai.struct.precompile.CalculateSeqSizes$.dataTypeBitsSize(CalculateSeqSizes.scala:92)
[info] at io.kaitai.struct.translators.CommonSizeOf$.getBitsSizeOfType(CommonSizeOf.scala:19)
[info] at io.kaitai.struct.translators.ExpressionValidator.byteSizeOfValue(ExpressionValidator.scala:132)
[info] at io.kaitai.struct.translators.ExpressionValidator.byteSizeOfValue(ExpressionValidator.scala:19)
[info] at io.kaitai.struct.translators.CommonMethods.translateAttribute(CommonMethods.scala:24)
[info] at io.kaitai.struct.translators.CommonMethods.translateAttribute$(CommonMethods.scala:17)
[info] at io.kaitai.struct.translators.ExpressionValidator.translateAttribute(ExpressionValidator.scala:19)
[info] at io.kaitai.struct.translators.ExpressionValidator.validate(ExpressionValidator.scala:65)
[info] at io.kaitai.struct.precompile.TypeValidator.validateValueInstance(TypeValidator.scala:108)
[info] ...
converted into this error:
[info] - expr_sizeof_value_dynamic3 *** FAILED ***
[info] expr_sizeof_value_dynamic3.ksy: /instances/body_sizeof/value:
[info] error: unable to derive sizeof of `Name(identifier(body))`: dynamic sized type
[info]
[info] expr_sizeof_value_dynamic3.ksy: /seq/0/id:
[info] warning: use `num_body` instead of `num`, given that it's only used as repeat count of `body` (see https://doc.kaitai.io/ksy_style_guide.html#attr-id)
[info] did not equal ? (SimpleMatchers.scala:34)
This error is fixed in https://github.com/kaitai-io/kaitai_struct_tests/pull/95
While current class naming is less than ideal, I wouldn't really agree to most of these changes:
- "Owned" / "Borrowed" is not really the straightforward, unfortunately. It implies certain semantics which might not exist in certain languages/scenarios, and is definitely not 1:1 mapping.
- "Array" => "Slice", at least in my opinion, is not a very good choice. "Slice" in this context is mostly golang terminology, and "slice" in e.g. Python means totally different thing, in JavaScript it's a method of Array, etc.
In other words, let's discuss such renames first.
- "Owned" / "Borrowed" is not really the straightforward, unfortunately. It implies certain semantics which might not exist in certain languages/scenarios, and is definitely not 1:1 mapping.
That semantic is used by the code generator. I have researched all the uses of types and don't see a single scenario where this is violated. Of course, languages with GC, such as Java, doesn't have that semantic reflected in the code, but that doesn't mean that it is missing. Moreover, the proposed names are already implicitly assumed in the code: just look at the presence of the property asNonOwning (==Borrowed). Also the type OwnedKaitaiStreamType already exists so it is just next logical step to unify all namings with similar meanings.
(In fact, I don't see much sense in dividing into Owned/Borrowed for KaitaiStructType/KaitaiStreamType/SliceType, because virtually all locations of use look like
case Owned... | Borrowed... => ...
In rare cases where this is not true you always can use
case t: ...Type if t.isOwning => ...
)
- "Array" => "Slice", at least in my opinion, is not a very good choice. "Slice" in this context is mostly golang terminology, and "slice" in e.g. Python means totally different thing, in JavaScript it's a method of Array, etc.
Actually, I had Rust in the mind and I think, that Rust terminology fits there very well. I don't see how the definition of slice in Python contradicts the intended name? Slice is part of an array (including the entire array). Slice assumes that this part can be extracted without copying, which may not be done in some languages, but this is only an implementation detail. If you want, let's refer to the definition from wikipedia:
In computer programming, array slicing is an operation that extracts a subset of elements from an array and packages them as another array, possibly in a different dimension from the original.
Common examples of array slicing are extracting a substring from a string of characters, the "ell" in "hello", extracting a row or column from a two-dimensional array, or extracting a vector from a matrix.
Depending on the programming language, an array slice can be made out of non-consecutive elements. Also depending on the language, the elements of the new array may be aliased to (i.e., share memory with) those of the original array.
In JavaScript, slice is a just a method declared in the Array by historical reasons, but it works not only with arrays, but with any array-like object, including slices (in terms of wikipedia).
In order to move forward I've removed the renames here, but I'll propose their in a new PR later
Last commit fixes ErrorMessagesSpec test for expr_sizeof_value_dynamic3:
[info] - expr_sizeof_value_dynamic3 *** FAILED ***
[info] scala.MatchError: CalcArrayType(Int1Type(false)) (of class io.kaitai.struct.datatype.DataType$CalcArrayType)
[info] at io.kaitai.struct.precompile.CalculateSeqSizes$.dataTypeByteSize(CalculateSeqSizes.scala:104)
[info] at io.kaitai.struct.precompile.CalculateSeqSizes$.dataTypeBitsSize(CalculateSeqSizes.scala:90)
[info] at io.kaitai.struct.translators.CommonSizeOf$.getBitsSizeOfType(CommonSizeOf.scala:19)
[info] at io.kaitai.struct.translators.ExpressionValidator.byteSizeOfValue(ExpressionValidator.scala:129)
[info] at io.kaitai.struct.translators.ExpressionValidator.byteSizeOfValue(ExpressionValidator.scala:18)
[info] at io.kaitai.struct.translators.CommonMethods.translateAttribute(CommonMethods.scala:24)
[info] at io.kaitai.struct.translators.CommonMethods.translateAttribute$(CommonMethods.scala:17)
[info] at io.kaitai.struct.translators.ExpressionValidator.translateAttribute(ExpressionValidator.scala:18)
[info] at io.kaitai.struct.translators.ExpressionValidator.validate(ExpressionValidator.scala:62)
[info] at io.kaitai.struct.precompile.TypeValidator.validateValueInstance(TypeValidator.scala:101)
[info] ...
@GreyCat, is it in the good state now for merge? I started again to documenting types to understand their purpose and remembered, that I already did that. I would appreciate if I could see these comments in the code rather than keep in mind.
@GreyCat, @generalmimon, I see that you have some activity in the project recently. Could you find a time to review my PRs, for example, this?