Skip to content

(basic) Boost serialization implementation#641

Open
ron0studios wants to merge 10 commits intogetml:mainfrom
ron0studios:boost-serialization
Open

(basic) Boost serialization implementation#641
ron0studios wants to merge 10 commits intogetml:mainfrom
ron0studios:boost-serialization

Conversation

@ron0studios
Copy link
Copy Markdown

A simple boost.serialization custom format for rfl.

Currently the basics are supported, but I feel adding patches will be outside the scope of this very first PR.

TODO:

  • docs
  • better/more rigorous tests (the current ones are just for validation). A test that showcases the Archive system of boost serialisation would be nice (&/<</>> overloads)
  • other boost formats (text_oarchive, xml_oarchive)
  • nested structs, rfl::Rename, unique/shared ptrs (box/ref), and a couple other rfl features.
  • better tests will probably reveal bugs :)

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces Boost.Serialization support to the library, implementing the necessary Reader, Writer, and Parser components to handle various data types. It also includes IO utilities for binary archives and a comprehensive suite of unit tests. The review feedback identifies a critical issue in the read_object function where errors were not being propagated, potentially leading to silent failures, and suggests a performance optimization to avoid redundant memory copies when reading from byte buffers.

@ron0studios ron0studios force-pushed the boost-serialization branch from 57031b0 to df96f7c Compare March 27, 2026 14:26
@ron0studios
Copy link
Copy Markdown
Author

undid the gemini code assist suggestion (57031b0)

@liuzicheng1987
Copy link
Copy Markdown
Contributor

@ron0studios let me know when this is ready for review

@liuzicheng1987
Copy link
Copy Markdown
Contributor

@ron0studios I just had a quick look. I think you're on the right track. I look forward to having this.

@ron0studios
Copy link
Copy Markdown
Author

@liuzicheng1987 Ready for review - everything but docs is written through. The remaining work was a matter of writing tests to see if some edge cases already worked out the box or not :)

@liuzicheng1987
Copy link
Copy Markdown
Contributor

@ron0studios , first of all, thanks for the contribution. This looks great and it inspired me to support https://github.com/USCiLab/cereal next.

I have a couple of minor remarks:

  1. As you have already observed, this still needs documentation. It needs to be mentioned in the README and it needs its own section in the general documentation. Feel free to write that using a coding agent of your choice (if you don't have one, I can do that).

  2. You need to update the Github Actions files to actually run your tests. I was able to run them locally and they pass, of course, but they need to run in an automated way.

  3. I think you could outsource some of the functionality (such as writer) into source files. Compile time is a major issue in a project like this and anything we can do to reduce it, we should do.

  4. It would also be cool to include this in the benchmarks. I am actually very curious how this performs in comparison to other serialization formats.

  5. I think we could add a few more tests and it wouldn't be hard to do...you could simply copy the tests for BSON, say, and they just search + replace bson with boost_serialization.

As a final remark, I am a bit concerned about security here, as there appears to be next to no validation...I wonder to what extent it would be possible to add a malicious file to get the code to do things it's not supposed. But this seems to be a general problem with Boost.Serialization and it's certainly not our job to fix it.

If you want to me to take over any of the points 1-5, I'd be happy to do so.

@ron0studios
Copy link
Copy Markdown
Author

@liuzicheng1987 thanks for reading through!

I'm happy to go through 1-5 :)

I have never tackled the security side of things before, so I'm not sure how helpful I can be on that front unfortunately.

I'll keep you posted!

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.

2 participants