Implement `doByteArrayNonLiteral` for Lua and C++ #281

Mingun · 2024-03-18T15:32:52Z

Fixes the test:

[info]     - lua:[0 + 1, 5].as<bytes> *** FAILED ***
[info]       scala.NotImplementedError: an implementation is missing
[info]       at scala.Predef$.$qmark$qmark$qmark(Predef.scala:344)
[info]       at io.kaitai.struct.translators.BaseTranslator.doByteArrayNonLiteral(BaseTranslator.scala:179)
[info]       at io.kaitai.struct.translators.BaseTranslator.doByteArrayNonLiteral(BaseTranslator.scala:28)
[info]       at io.kaitai.struct.translators.CommonArraysAndCast.doByteArray(CommonArraysAndCast.scala:85)
[info]       at io.kaitai.struct.translators.CommonArraysAndCast.doCastOrArray(CommonArraysAndCast.scala:62)
[info]       at io.kaitai.struct.translators.CommonArraysAndCast.doCastOrArray$(CommonArraysAndCast.scala:53)
[info]       at io.kaitai.struct.translators.BaseTranslator.doCastOrArray(BaseTranslator.scala:28)
[info]       at io.kaitai.struct.translators.BaseTranslator.translate(BaseTranslator.scala:147)
[info]       at io.kaitai.struct.translators.AbstractTranslator.translate(AbstractTranslator.scala:25)
[info]       at io.kaitai.struct.translators.AbstractTranslator.translate$(AbstractTranslator.scala:25)
[info]       ...

Looking at existing implementations for other languages I suspect that the missing implementation in both cases was just an oversight when doByteArrayNonLiteral was added.

C++ test

[info]     - cpp_stl:[0 + 1, 5].as<bytes> *** FAILED ***
[info]       scala.NotImplementedError: an implementation is missing
[info]       at scala.Predef$.$qmark$qmark$qmark(Predef.scala:344)
[info]       at io.kaitai.struct.translators.BaseTranslator.doByteArrayNonLiteral(BaseTranslator.scala:179)
[info]       at io.kaitai.struct.translators.BaseTranslator.doByteArrayNonLiteral(BaseTranslator.scala:28)
[info]       at io.kaitai.struct.translators.CommonArraysAndCast.doByteArray(CommonArraysAndCast.scala:85)
[info]       at io.kaitai.struct.translators.CommonArraysAndCast.doCastOrArray(CommonArraysAndCast.scala:62)
[info]       at io.kaitai.struct.translators.CommonArraysAndCast.doCastOrArray$(CommonArraysAndCast.scala:53)
[info]       at io.kaitai.struct.translators.BaseTranslator.doCastOrArray(BaseTranslator.scala:28)
[info]       at io.kaitai.struct.translators.BaseTranslator.translate(BaseTranslator.scala:147)
[info]       at io.kaitai.struct.translators.AbstractTranslator.translate(AbstractTranslator.scala:25)
[info]       at io.kaitai.struct.translators.AbstractTranslator.translate$(AbstractTranslator.scala:25)
[info]       ...

now converted to another failure

[info]     - cpp_stl:[0 + 1, 5].as<bytes> *** FAILED ***
[info]       java.lang.RuntimeException: C++ literal arrays are not implemented yet
[info]       at io.kaitai.struct.translators.CppTranslator.doArrayLiteral(CppTranslator.scala:114)
[info]       at io.kaitai.struct.translators.CppTranslator.doByteArrayNonLiteral(CppTranslator.scala:121)
[info]       at io.kaitai.struct.translators.CppTranslator.doByteArrayNonLiteral(CppTranslator.scala:15)
[info]       at io.kaitai.struct.translators.CommonArraysAndCast.doByteArray(CommonArraysAndCast.scala:85)
[info]       at io.kaitai.struct.translators.CommonArraysAndCast.doCastOrArray(CommonArraysAndCast.scala:62)
[info]       at io.kaitai.struct.translators.CommonArraysAndCast.doCastOrArray$(CommonArraysAndCast.scala:53)
[info]       at io.kaitai.struct.translators.BaseTranslator.doCastOrArray(BaseTranslator.scala:28)
[info]       at io.kaitai.struct.translators.BaseTranslator.translate(BaseTranslator.scala:146)
[info]       at io.kaitai.struct.translators.TranslatorSpec.$anonfun$runTest$4(TranslatorSpec.scala:789)
[info]       at org.scalatest.OutcomeOf.outcomeOf(OutcomeOf.scala:85)
[info]       ...

which is because of unimplemented feature (tracked in kaitai-io/kaitai_struct#932).

Note, that it would be worth to test targets with options, such as C++, with different set of options. Right now only default options are used.

generalmimon · 2024-03-20T11:52:55Z

shared/src/main/scala/io/kaitai/struct/translators/CppTranslator.scala

+  override def doByteArrayNonLiteral(values: Seq[Ast.expr]): String =
+    doArrayLiteral(DataType.Int1Type(false), values)


I don't think this is correct. It looks like you're generating a true array literal here, not a byte array literal. Byte arrays are translated as std::string in C++ (see doByteArrayLiteral right above).

If we consider a real-world example where this would be used, which is the EDID spec - hardware/edid.ksy:261-268:

instances: mfg_id_ch1: value: '(mfg_bytes & 0b0111110000000000) >> 10' mfg_id_ch2: value: '(mfg_bytes & 0b0000001111100000) >> 5' mfg_id_ch3: value: '(mfg_bytes & 0b0000000000011111)' mfg_str: value: '[mfg_id_ch1 + 0x40, mfg_id_ch2 + 0x40, mfg_id_ch3 + 0x40].as<bytes>.to_s("ASCII")'

I guess the code generation in C++ could look more like this:

std::string bytes_literal_t::mfg_str() { if (f_mfg_str) return m_mfg_str; m_mfg_str = kaitai::kstream::bytes_to_str(std::string({ mfg_id_ch1() + 0x40, mfg_id_ch2() + 0x40, mfg_id_ch3() + 0x40 }), "ASCII"); f_mfg_str = true; return m_mfg_str; }

Note the std::string.

Unfortunately, doByteArrayLiteral should produce an expression, but without initializer lists it is possible to use only statement to create a byte array and then std::string from it:

const char bytes[] = { char(mfg_id_ch1() + 0x40), char(mfg_id_ch2() + 0x40), char(mfg_id_ch3() + 0x40), }; std::string(bytes, sizeof(bytes));

https://godbolt.org/z/1Y8Ec9f7K

Because anyway C++98 target is not fully implemented, I've implemented only initializer_list solution (so only since C++11).

In all fairness, there's nothing terribly wrong with getting our CppTranslator closer to GoTranslator in a sense that it might also use temporary variables allocated on stack to make things like that separate const char bytes[] = { ... }; work for C++98. We already do that for Go, we will likely do that for C.

Indeed we should do that, but not in this PR

generalmimon · 2024-03-20T11:59:48Z

shared/src/main/scala/io/kaitai/struct/translators/LuaTranslator.scala

+  override def doByteArrayNonLiteral(values: Seq[Ast.expr]): String =
+    "{" + values.map(translate).mkString(", ") + "}"


This also doesn't look like a byte array.

Note that the runtime type you generate here in doByteArrayNonLiteral must be the same as the runtime type generated in doByteArrayLiteral. This is not the case with this implementation, as you can check at https://tio.run/##yylN/P@/oCgzr0SjpLIgVUMpxsDMNMbI1DTGwNxUSVOTC0mu2sxURwEopaNgblqrqfn/PwA:

print(type("\065\255\075")) -- => string print(type({65, 255, 75})) -- => table

Ok, the Lua stuff was simple

GreyCat · 2024-03-24T09:53:12Z

shared/src/main/scala/io/kaitai/struct/translators/CppTranslator.scala

+  override def doByteArrayNonLiteral(values: Seq[Ast.expr]): String = {
+    // It is assumed that every expression produces integer in the range [0; 255]
+    if (config.cppConfig.useListInitializers) {
+      "std::string({" + values.map(value => s"char(${translate(value)})").mkString(", ") + "})"


char(x) is what's called functional-style cast. It's not C-style cast, but it is equally implicit and can mean a lot of things.

I wonder if using static_cast<char>(...) will make more sense here?

Now static_cast<char>(...) is used

@Mingun:

Now static_cast<char>(...) is used

According to the latest commit 69856bb on this PR, this is not the case:

kaitai_struct_compiler/shared/src/main/scala/io/kaitai/struct/translators/CppTranslator.scala

Line 125 in 69856bb

"std::string({" + values.map(value => s"char(${translate(value)})").mkString(", ") + "})"

Apparently, static_cast<char>(...) was reverted back to char(...) in the last force push: 222472e..69856bb

Oops... now really fixed

GreyCat

Generally looks good, let's just close on whether we want static_cast or functional-style casts.

jvm/src/test/scala/io/kaitai/struct/translators/TranslatorSpec.scala

shared/src/main/scala/io/kaitai/struct/translators/CppTranslator.scala

Mingun · 2024-04-05T16:06:17Z

@generalmimon, what is your conclusion?

…f `bytes` type Expressions of this type are created using: - omitting the `type:` field in attributes and parse instances - declaring parameters with `type: bytes` - using `.as<bytes>` cast Fixes test ``` [info] - lua:[0 + 1, 5].as<bytes> *** FAILED *** [info] scala.NotImplementedError: an implementation is missing [info] at scala.Predef$.$qmark$qmark$qmark(Predef.scala:344) [info] at io.kaitai.struct.translators.BaseTranslator.doByteArrayNonLiteral(BaseTranslator.scala:179) [info] at io.kaitai.struct.translators.BaseTranslator.doByteArrayNonLiteral(BaseTranslator.scala:28) [info] at io.kaitai.struct.translators.CommonArraysAndCast.doByteArray(CommonArraysAndCast.scala:85) [info] at io.kaitai.struct.translators.CommonArraysAndCast.doCastOrArray(CommonArraysAndCast.scala:62) [info] at io.kaitai.struct.translators.CommonArraysAndCast.doCastOrArray$(CommonArraysAndCast.scala:53) [info] at io.kaitai.struct.translators.BaseTranslator.doCastOrArray(BaseTranslator.scala:28) [info] at io.kaitai.struct.translators.BaseTranslator.translate(BaseTranslator.scala:147) [info] at io.kaitai.struct.translators.AbstractTranslator.translate(AbstractTranslator.scala:25) [info] at io.kaitai.struct.translators.AbstractTranslator.translate$(AbstractTranslator.scala:25) [info] ... ```

…f `bytes` type Expressions of this type are created using: - omitting the `type:` field in attributes and parse instances - declaring parameters with `type: bytes` - using `.as<bytes>` cast Ghostly fixes test ``` [info] - cpp_stl:[0 + 1, 5].as<bytes> *** FAILED *** [info] scala.NotImplementedError: an implementation is missing [info] at scala.Predef$.$qmark$qmark$qmark(Predef.scala:344) [info] at io.kaitai.struct.translators.BaseTranslator.doByteArrayNonLiteral(BaseTranslator.scala:179) [info] at io.kaitai.struct.translators.BaseTranslator.doByteArrayNonLiteral(BaseTranslator.scala:28) [info] at io.kaitai.struct.translators.CommonArraysAndCast.doByteArray(CommonArraysAndCast.scala:85) [info] at io.kaitai.struct.translators.CommonArraysAndCast.doCastOrArray(CommonArraysAndCast.scala:62) [info] at io.kaitai.struct.translators.CommonArraysAndCast.doCastOrArray$(CommonArraysAndCast.scala:53) [info] at io.kaitai.struct.translators.BaseTranslator.doCastOrArray(BaseTranslator.scala:28) [info] at io.kaitai.struct.translators.BaseTranslator.translate(BaseTranslator.scala:147) [info] at io.kaitai.struct.translators.AbstractTranslator.translate(AbstractTranslator.scala:25) [info] at io.kaitai.struct.translators.AbstractTranslator.translate$(AbstractTranslator.scala:25) [info] ... ``` (actually, test is failing because option `useListInitializers` in C++ backend is not set in tests and code generation without it is not unimplemented right now)

generalmimon

@Mingun Thanks!

generalmimon requested changes Mar 20, 2024

View reviewed changes

Mingun force-pushed the impl-do-byte-array-non-literal branch from 1c22828 to 3b14bb0 Compare March 20, 2024 16:08

Mingun requested a review from generalmimon March 20, 2024 16:14

Mingun force-pushed the impl-do-byte-array-non-literal branch 3 times, most recently from e4500bc to d30fd3a Compare March 22, 2024 15:34

GreyCat reviewed Mar 24, 2024

View reviewed changes

GreyCat approved these changes Mar 24, 2024

View reviewed changes

Mingun force-pushed the impl-do-byte-array-non-literal branch from d30fd3a to 73ec6d8 Compare March 30, 2024 08:45

GreyCat reviewed Mar 30, 2024

View reviewed changes

jvm/src/test/scala/io/kaitai/struct/translators/TranslatorSpec.scala Outdated Show resolved Hide resolved

Mingun force-pushed the impl-do-byte-array-non-literal branch from 73ec6d8 to 5b78d53 Compare March 30, 2024 09:15

generalmimon reviewed Mar 30, 2024

View reviewed changes

shared/src/main/scala/io/kaitai/struct/translators/CppTranslator.scala Outdated Show resolved Hide resolved

generalmimon reviewed Mar 30, 2024

View reviewed changes

shared/src/main/scala/io/kaitai/struct/translators/CppTranslator.scala Outdated Show resolved Hide resolved

Mingun force-pushed the impl-do-byte-array-non-literal branch 2 times, most recently from 3f574b5 to b0c2a5d Compare March 30, 2024 10:18

Mingun requested a review from generalmimon April 4, 2024 15:37

Mingun force-pushed the impl-do-byte-array-non-literal branch from b0c2a5d to 717ee21 Compare April 7, 2024 20:03

Mingun force-pushed the impl-do-byte-array-non-literal branch 2 times, most recently from 222472e to 69856bb Compare July 16, 2024 14:20

Mingun force-pushed the impl-do-byte-array-non-literal branch from 69856bb to 3605b6a Compare August 25, 2024 16:57

Mingun added 2 commits September 8, 2024 08:49

Mingun force-pushed the impl-do-byte-array-non-literal branch from 3605b6a to c8bd711 Compare September 8, 2024 03:49

generalmimon added 2 commits October 3, 2024 01:26

C++: improve "not yet implemented" error messages

208cf53

CppTranslator: language fixes in code comment

cd3c0e3

generalmimon approved these changes Oct 2, 2024

View reviewed changes

generalmimon merged commit 542b241 into kaitai-io:master Oct 2, 2024
1 of 4 checks passed

Mingun deleted the impl-do-byte-array-non-literal branch October 3, 2024 09:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `doByteArrayNonLiteral` for Lua and C++ #281

Implement `doByteArrayNonLiteral` for Lua and C++ #281

Mingun commented Mar 18, 2024 •

edited

Loading

generalmimon Mar 20, 2024

Mingun Mar 20, 2024 •

edited

Loading

GreyCat Mar 24, 2024

Mingun Mar 30, 2024

generalmimon Mar 20, 2024 •

edited

Loading

Mingun Mar 20, 2024

GreyCat Mar 24, 2024

Mingun Mar 30, 2024

generalmimon Aug 25, 2024 •

edited

Loading

Mingun Aug 25, 2024

GreyCat left a comment

Mingun commented Apr 5, 2024

generalmimon left a comment

		override def doByteArrayNonLiteral(values: Seq[Ast.expr]): String =
		doArrayLiteral(DataType.Int1Type(false), values)

		override def doByteArrayNonLiteral(values: Seq[Ast.expr]): String =
		"{" + values.map(translate).mkString(", ") + "}"

Implement doByteArrayNonLiteral for Lua and C++ #281

Implement doByteArrayNonLiteral for Lua and C++ #281

Conversation

Mingun commented Mar 18, 2024 • edited Loading

generalmimon Mar 20, 2024

Choose a reason for hiding this comment

Mingun Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

GreyCat Mar 24, 2024

Choose a reason for hiding this comment

Mingun Mar 30, 2024

Choose a reason for hiding this comment

generalmimon Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Mingun Mar 20, 2024

Choose a reason for hiding this comment

GreyCat Mar 24, 2024

Choose a reason for hiding this comment

Mingun Mar 30, 2024

Choose a reason for hiding this comment

generalmimon Aug 25, 2024 • edited Loading

Choose a reason for hiding this comment

Mingun Aug 25, 2024

Choose a reason for hiding this comment

GreyCat left a comment

Choose a reason for hiding this comment

Mingun commented Apr 5, 2024

generalmimon left a comment

Choose a reason for hiding this comment

Implement `doByteArrayNonLiteral` for Lua and C++ #281

Implement `doByteArrayNonLiteral` for Lua and C++ #281

Mingun commented Mar 18, 2024 •

edited

Loading

Mingun Mar 20, 2024 •

edited

Loading

generalmimon Mar 20, 2024 •

edited

Loading

generalmimon Aug 25, 2024 •

edited

Loading