-
Notifications
You must be signed in to change notification settings - Fork 799
Initial support for concatenated archives, and memory fixes #88
New issue
< 8000 details-dialog class="Box Box--overlay d-flex flex-column anim-fade-in fast overflow-auto" aria-label="Sign up for GitHub">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
base: master
Are you sure you want to change the base?
Conversation
This tests the case where multiple archives are concatenated together in a single stream. For example this can happen if multiple archives are being streamed from a pipe. The JSON and XML variants of the test are disabled, because they don't pass the test.
We might be moving away from both RapidXML and RapidJSON in the next release (see #82 and #83), which would affect both changes you've proposed here, though I'm not entirely sure how significant the impact would be. Can you describe the situation in which you ran into issues using the current setNextName implementation? |
The The resolution is to copy the string inside With that change in place for This branch is kind-of like a buffet - take from it what you want. With respect to the concatenation patches. I would be keen to see the test case included at the very least. The patch to Finally, 179d839 should be applied straight away. This is a simple fix for a memory bug caused by a temporary string going out of scope. |
I merged 179d839 into develop. |
@AzothAmmo thanks for pulling that commit. - though what should we do about the others? Is there somewhere regular contributors meet? the mailing list or an IRC channel so that we can discuss these things? |
We recently made a mailing list: https://groups.google.com/forum/#!forum/cerealcpp. It hasn't been used much yet. As far as the other issues, I think they should be investigated in tandem with migrating to PugiXML, especially your desire to be able to have more than one archive in a given stream. I think the migration itself will likely not be a large amount of work. |
This branch does the following...
Memory management fixes
Fixes some memory management issues. Currently cereal has a mixture of strings handled with
std::string
and strings handled withchar*
. The use of raw-pointers leads to some counter-intuitive behaviour. For example:Typically the caller would expect this to work, because setNextName would take a copy of the string - but it doesn't. Therefore, in this case there will be a memory error after
s
goes out of scope. The first 5 patches address this issue by replacing some of cereal's raw pointers withstd::string
s.Support for concatenated archives
The JSON and XML input archives currently read in the whole of an input stream until they hit the end of the file. In some cases this is undesirable. For example a file may contain more data after an archive, or it may be received via some kind of socket stream, or via a UNIX pipe. In these cases it is incorrect to read to the end of the file. The correct behavior is to read to the end of the XML or JSON document.
This branch addresses this issue by adding a test case to show this issue. It writes two archives into a single stream, then checks that they are both read out correctly.
Following the test case, I have modified
XMLInputArchive
to add support.Currently if two XML archives are written to an output stream the following output will be generated:
Here we are outputting one archive with the string
Apples\n\nPears
and another with the stringOranges\n\nLemons
. cereal's XML output will insert an empty new-line after each XML document.Currently RapidXML does not provide a mechanism by which it can read only up to the end of the document, so my modification reads until it finds an empty line, then attempts to parse the accumulated XML. If the parse fails, this indicates that we have hit an empty line inside the XML document and the we should continue reading. The drawback of this approach is that the document must be re-parsed every time the input stream emits a blank line, which might become problematic in an XML document containing many empty lines.
There is still some work remaining to fix
JSONInputArchive
similarly to pass the test. It may be possible to do this via RapidJSON's internal stream support, or by counting the the number of{
and}
braces, until the tally returns to zero.