-
Notifications
You must be signed in to change notification settings - Fork 755
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
Unordered loading (XML and JSON) not working when types have a wrapper (e.g. ptr_wrapper) #42
Comments
I should mention I am on the head of "develop" |
Minimal test included!
|
Stack Overflow question for points if you like: http://stackoverflow.com/questions/20940919/c11-cereal-load-and-allocate-not-loading-correctly |
New minimal test! It seems like even the most basic example fails. I was looking for coverage in unit tests and I didn't see any instance of load_and_allocate being used.* (edit: it seems this stuff is in the sandbox cpp files, but in these examples there are no member values being loaded and so this case would not be hit.) I tried this with XML archives as well with the same problem (though instead the exception throws from xml.hpp startNode()).
|
Definitely a bug on our part introduced with the out of order loading. Probably also exists for XML archives. The bug is exactly in the search function where you described, and the issue is that we have a few special wrappers we sometimes use that introduce additional nodes into the JSON/XML trees that are cereal-specific metadata and not really relevant to a user looking at the JSON/XML. Toying around two ideas to fix this in my head right now. One is to adjust the search function or introduce some state to the archive such that it knows when it is inside of a wrapper. The other solution would be to split cereal metadata into an entirely different node than actual data saved in a JSON/XML archive. So for your example (slightly changed to return a {
"cereal_data": {
"nvp1": {
"derivedMember": 4,
"base": {
"name": "TestName",
"baseMember": 0
}
}
},
"cereal_metadata_do_not_edit":
{
"nvp1": {
"polymorphic_id": 2147483649,
"polymorphic_name": "DerivedClass",
"ptr_id": 2147483649
}
"nvp2": {
"polymorphic_id": 1,
"ptr_id": 1
}
}
} I'll have to think this over. The second approach would appeal to some others (e.g. DrAWolf) who have been asking for the text based archives to be more readable, and this would solve that issue more or less. We would likely have some decreased performance though. |
Yeah, my initial reaction was that we would have to dig the "data" part out of the archive and pass that through in our load_andor_allocate calls in line 143 and 166 in memory.hpp:
But I'm not really familiar with how you would do that. I can see the merit to breaking out the structure as well, but I suspect that's a riskier and bigger task. |
I'm not sure there is actually any dependence on |
Yeah, I'm kind of thrashing for an explanation because I am just not very familiar with this code base and in fact am kind of new to enable_if and template metaprogramming stuff that you guys employ a lot of. It's a very nested code base so I could be way off. I guess I just see the memory.hpp save methods for PtrWrapper explicitly save data in a "data" field, but never explicitly pull the "data" field out in the corresponding load methods. |
For now, to work around the issue I added a line 144 in memory.hpp (as it appears on line 168 in the case of no load_and_allocate which means that there is a default constructor.)
I will simply avoid using the load_and_allocate archive directly and will use my serialization functions. In my load_and_allocate method I will construct an object with "default" like information. |
Hey, just as I saw the relation to my suggestions, you already mention my name. |
The optimal solution would be a loader that could identify/recognize the polymorphic type - by looking at the entries present/given in a JSON object. |
Actually looking at this the issue is with load_and_allocate, which does not enter into the data node created by the ptr_wrapper. In regards to the possible splitting of the data in the text archives, this doesn't duplicate any data except for NVP names to index the information. One node would contain data the other metadata. There is no way for us to avoid including this metadata somewhere. I think the only two good solutions are to keep it as is (which I don't think is such a big deal), or the alternative path. |
load_and_allocate did not properly enter into the 'data' NVP that the ptr_wrapper creates for unique/shared ptr. When loading these types, we now go through a wrapper struct to force entry into an extra node to resolve this issue. Changes to unittests are for an issue compiling with g++-4.7.3 under Ubuntu where steady_clock::now() is not defined for some reason
Marking as closed let me know if issue persists. |
I've resolved quite a few of the issues I was having, but I think I may have discovered an actual bug in this case. See the following pastebin for my textures.h header which has most of the relevant code.
http://pastebin.com/emW9HBSR
If you search for !ERROR! in the above pastebin you can see the location of the issue. Basically in my load_and_allocate method when I try to read from the archive it is looking directly inside "ptr_wrapper" at the "id" and "data" fields instead of inside the "ptr_wrapper"."data" field which contains the actual class values.
I know that it's looking at "id" and "data" because I modified "search" in json.hpp to look like this:
This is the method that throws, and it throws just after printing:
!repeat == id
!repeat == data
Which means it is searching for "repeat" and compares against "id" and "data".
I'm trying to run this test function:
Here's what my std::cout << stream.str() << std::endl; line spits out:
The text was updated successfully, but these errors were encountered: