Loki-Astari / ThorsMongo

C++ MongoDB API and BSON/JSON Serialization library
GNU General Public License v3.0
316 stars 72 forks source link

Support use of shared_ptr (for polymorphic types) #54

Closed scgroot closed 1 year ago

scgroot commented 4 years ago

Is your feature request related to a problem? Please describe. Currently when using polymorphic types, only unique_ptr's work. The documentation on https://lokiastari.com/ThorsSerializer/#Pointers also states that they are not supported. I think currently they already work, just not in combination with polymoprhic types.

Sometimes they are convenient though.

Describe the solution you'd like Support shared_ptr's, like unique_ptr.

Describe alternatives you've considered Not support them?:) Support seems already (partly) there in the code? So I'm not sure if they are intentionally left out, or just never really needed so far?

Additional context By adding the two missing template specializations for shared_ptr in Traits.h, line 488 and in Serialize.tpp line 281 it seems to work. (Using the header only branch)

Loki-Astari commented 4 years ago

The reason I have not added them was simply I have not needed them (and its slightly more complex than you initially think).

The trivial way of doing this is easy, simply treat them the same as unique_ptr. But shared_ptr has the extra property that they can be referred to from multiple locations (and there can be weak pointers).

The trouble is not the serialization but because it can be referred to from several locations. When you serialize these (treating them as unique_ptr like) objects they will be serialized multiple times if they are referred to from multiple locations. The recombination when de-serializing the JSON will result in multiple objects rather than a single shared object.

I am am more than willing to accept pull requests. Though they are required to pass all the unit tests and vera tests (which usually requires you to check by using the master branch). Though if there are only minor changes I'll probably just fix it. I also have a requirement of 80% code coverage (which should be easily achieved by copying the unique_ptr tests).

scgroot commented 4 years ago

You're right that in json there is not really a shared pointer concept of course. Unique is more correct in that sense.

Browsing trough the code I see that shared_ptr is already supported in a lot of situations, just not for polymorphism. Maybe it's nice to support is completely then, instead of half.

I'll create a pull request for it! You can close this if you agree of course.

Loki-Astari commented 4 years ago

Thanks for the pull request. I am away for a couple of days, but I should be able to give it a look at the weekend.

scgroot commented 4 years ago

Ok, enjoy!

JakkuSakura commented 4 years ago

https://github.com/Loki-Astari/ThorsSerializer/issues/61#issuecomment-592554424 Today I implemented a similay functionality: wrap POD to make them polymorphic. It's very tough, though.

Loki-Astari commented 1 year ago

This has now been merged.