Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial support for serialization #30

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jchv
Copy link

@jchv jchv commented Sep 7, 2019

Implements write support for some of the primitives, at least enough to write basic seqs. The API had to be shifted over a bit, but it should be API compatible, though this certainly breaks ABI compatibility. Does this matter? Is there any kind of major version bump to do? I do not think there is a practical way to avoid the ABI compatibility issue.

Currently the code here seems to build just fine, but I'm putting this PR in draft mode since I desire to do more testing and make sure it compiles with existing generated code.

Fixes #29.

jchv added 2 commits September 2, 2019 20:54
This is part of adding basic write support to the C++ runtime.
@jchv jchv force-pushed the basic-write-support-2 branch from d853591 to aea9971 Compare September 8, 2019 00:21
jchv added 3 commits September 7, 2019 19:04
This should be enough to enable serialization of very basic seqs for the
C++ code generator in the future.
@jchv jchv force-pushed the basic-write-support-2 branch from aea9971 to b2f126c Compare September 8, 2019 02:05
@jchv
Copy link
Author

jchv commented Sep 8, 2019

Because unit tests hardcode the paths to the cpp files in the runtime, it is necessary to patch the test CMakeLists files.

diff --git a/spec/cpp_stl_11/CMakeLists.txt b/spec/cpp_stl_11/CMakeLists.txt
index 1176dab..8b411e6 100644
--- a/spec/cpp_stl_11/CMakeLists.txt
+++ b/spec/cpp_stl_11/CMakeLists.txt
@@ -26,7 +26,9 @@ set(PREREQ_SOURCES
 )
 
 set(RUNTIME_SOURCES
-       ${RUNTIME_SRC_PATH}/kaitai/kaitaistream.cpp
+       ${RUNTIME_SRC_PATH}/kaitai/kio.cpp
+       ${RUNTIME_SRC_PATH}/kaitai/kistream.cpp
+       ${RUNTIME_SRC_PATH}/kaitai/kostream.cpp
 )
 
 add_executable (ks_tests
diff --git a/spec/cpp_stl_98/CMakeLists.txt b/spec/cpp_stl_98/CMakeLists.txt
index f0ae8e8..069f867 100644
--- a/spec/cpp_stl_98/CMakeLists.txt
+++ b/spec/cpp_stl_98/CMakeLists.txt
@@ -22,7 +22,9 @@ set(PREREQ_SOURCES
 )
 
 set(RUNTIME_SOURCES
-       ${RUNTIME_SRC_PATH}/kaitai/kaitaistream.cpp
+       ${RUNTIME_SRC_PATH}/kaitai/kio.cpp
+       ${RUNTIME_SRC_PATH}/kaitai/kistream.cpp
+       ${RUNTIME_SRC_PATH}/kaitai/kostream.cpp
 )
 
 add_executable (ks_tests

For me locally, there are some tests that do not compile and one test that fails at master. However, that happens exactly the same regardless of whether I am using this version of the runtime or not, and are evidently fairly unrelated errors, so this PR is probably not broken.

Alternative solutions:

  • kaitaistream.cpp file that includes the other cpp files.
  • Linking to the other CMake project in a more regular fashion (may be ugly)
  • Making C++ runtime header-only (I actually wonder why it isn't? This would definitely fix the ABI breakage problem.)

kaitai/kio.cpp Show resolved Hide resolved
@JPenuchot
Copy link

Hello,

Any hope to see these merged in Kaitai someday ? This would pave the way for fixing an old issue in Mixxx (here).

Regards,
Jules

@jchv
Copy link
Author

jchv commented Sep 26, 2021

@JPenuchot: For what it's worth, this would only implement runtime primitives necessary for adding basic serialization support into Kaitai Struct for C++. Merging this would be a start, but we would also need to add support into the Kaitai Struct code generation portion as well.

@eoan-ermine
Copy link

@jchv @GreyCat

So, what's with this pull request? I saw that the code was reviewed, but it was not merged after that. Is there some kind of blocker?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Design for serialization API
4 participants