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

Design for serialization API #29

Open
jchv opened this issue Aug 31, 2019 · 4 comments · May be fixed by #30
Open

Design for serialization API #29

jchv opened this issue Aug 31, 2019 · 4 comments · May be fixed by #30

Comments

@jchv
Copy link

jchv commented Aug 31, 2019

Hey. I'm working on trying to add basic write functionality to the runtime, in preparation for hopefully eventually contributing to the serialization issue.

Unfortunately, it seems like this is actually pretty tough to do without breaking changes.

Right now the C++ API has a constructor like this:

    /**
     * Constructs new Kaitai Stream object, wrapping a given std::istream.
     * \param io istream object to use for this Kaitai Stream
     */
    kstream(std::istream* io);

It uses the actual std::istream* passed in, which means that we kind of need to keep an std::istream* to remain fully semantically compatible.

I think there's basically four approaches:

  1. Take the streambuf from the incoming std::istream* and pass it to an std::iostream. This would enable you to do the same for an incoming std::ostream*. In theory I think this does most of what you want, but it almost certainly will behave differently since the passed in stream is not touched directly. In addition, there are several const member functions that call non-const methods on m_io, so if you make m_io an std::iostream (not a pointer) it must be marked mutable to maintain the same API (or const-casted everywhere, etc.)

  2. Store the incoming std::istream* as both std::ios* and std::istream*, and have an additional pointer for std::ostream*. On input streams, the std::ostream* would be NULL and on output streams, the std::istream* would be NULL. This sucks because we need additional checks in order to prevent segfaults, since now any read/write call may hit a null pointer if kstream was created on the wrong type of iostream.

  3. Make a new class for output streams, perhaps kaitai::kostream. Perhaps have a base class for some of the shared functionality. Although this solution is not terrible, because kaitai::kstream exists today, it would likely always have to exist or at least be aliased to something like kaitai::kistream.

  4. Break the API and only take in a streambuf* or something similarly more generic. Is this even an option?

I don't think input and output streams have a ton of overlap, so maybe this is not a huge issue; but it does seem like this requires some thought...

@GreyCat
Copy link
Member

GreyCat commented Aug 31, 2019

In general, I believe the consensus was around that we'll have two modes of code generation: "read-only" (i.e. only for parsing, as it is) and "read-write" (i.e. for both parsing + serialization, so it's natural that it will work on iostream, not istream).

This distinction implies that:

  • KaitaiStruct interface splits into KaitaiStructReadable (with _read()) and KaitaiStructWritable (with _write())
  • KaitaiStream should likely follow the underlying language stream semantics. Given that C++ stream splits between istream / ostream / iostream, probably it will make sense to mirror that into kistream, kostream, kiostream?

@jchv
Copy link
Author

jchv commented Sep 1, 2019

Thanks for responding.

  • KaitaiStream should likely follow the underlying language stream semantics. Given that C++ stream splits between istream / ostream / iostream, probably it will make sense to mirror that into kistream, kostream, kiostream?

Hmm, OK. So then maybe something like:

  • kio: Base class with common bits. Takes std::ios*. class kio
  • kistream: Input stream class. Takes std::istream*. Derives from kio. class kistream : public virtual kio
  • kostream: Output stream class. Takes std::ostream* Derives from kio. class kostream : public virtual kio
  • kiostream: Input/output class. Takes std::iostream*. class kiostream : public kistream, public kostream.
  • kstream: Compatibility typedef for kistream.

Because iostream derives from ostream and istream, and all of them derive from ios, it should be possible to pass the pointer all the way down. That way the base class can do things like twiddle the exception flags on and off.

@GreyCat
Copy link
Member

GreyCat commented Sep 3, 2019

Seems to be a good compromise. I'm not sure that maintaining compatibility with kstream is worth it — but given that in C++ exactly it seems to be used manually far more often that in other languages, may be it's worth going that extra mile too.

@jchv jchv linked a pull request Sep 7, 2019 that will close this issue
@computerquip
Copy link

I see that this is getting quite old but I still check it every few months because I want this to be a thing. I see in the pull request that code generation in the compiler is still a concern. Is there a list of things that need to be finished or hasn't been worked on? I wouldn't mind trying to fill in some gaps.

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 a pull request may close this issue.

3 participants