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

Extend Stream library to support non-trivially-copyable objects #277

Open
DanRStevens opened this issue Mar 7, 2019 · 5 comments
Open

Comments

@DanRStevens
Copy link
Member

DanRStevens commented Mar 7, 2019

I noticed there are a number of structs being used for serialization, but due to complex members, such as std::vector, they are not std::is_trivially_copyable. As such, they can not be automatically serialized.

To get around this, such objects often have a custom Write(Stream::Writer&) style method, which is manually called. I was thinking perhaps the Stream library could be updated to support auto calling such a member function on non-trivially-copyable objects. This would make serialization of such complex objects more consistent with traditional trivially-copyable objects.

Example:

struct ComplexObjectType {
  std::vector<int> nonTrivialField;  // Internally the vector may have pointers to external buffers

  Write(Stream::Writer& writer);
};

void f(Stream::Writer& writer) {
  ComplexObjectType object;
  writer.Write(object);  // Proposed upgrade would auto call: object.Write(writer)
}

On it's own, this small syntax adjustment doesn't buy much. It merely swaps the order of the writer and the object. However, combine this with a small update to vector writes, and we would be able to automatically serialize a std::vector<T> of non-trivial types. See the proposal in Issue #278.

@DanRStevens
Copy link
Member Author

This article has some details about how to detect the presence of a given member:
https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Member_Detector

I was hoping there would be a built in type_traits method, but I'm not seeing an exact match. The closest looking one I see is is_member_function_pointer. Maybe it can be used.

@Brett208
Copy link
Member

I generally support this and #278.

In map, we have both a WriteMap and WriteSavedGame. I'm guessing that class would not be a candidate for this improvement unless we separated into two classes, Map and SavedGame.

Also, for Bitmap, we have named the write function WriteIndexed. I like this because it explicitly tells the user they cannot write a non-indexed bitmap. This function could just be renamed to write, and a comment added about only supporting index bitmaps.

In C# you can decorate functions although I haven't researched the concept in C++. There might be a way to decorate a different named function to serve as the 'Write' function, although I don't know if we want to support it or not. It starts adding more complexity to the project since less people will be familiar with decorations.

The proposed syntax will certainly improve the readability when reading/writing.

-Brett

@DanRStevens
Copy link
Member Author

That's a good point, yes. I suppose SavedGame could be made a subclass of Map. Though I'm not sure if that's something we want to do. The other thought is, the class itself knows if it has only map data, or full saved game data, so it can choose which to write. Though that doesn't play well with the idea of extracting a map from saved game data.

Another possibility is to allow specifying a serialization method. Example: an Image class could serialize as BMP, JPEG, PNG, TIFF, etc.

For Bitmap, maybe the class should be named IndexedBitmap? Or if there is a desire to extend it to support other bitmap types, to just name the methods accordingly, and throw a not supported error if used outside of its current design.

Sounds like this idea may need a bit of refinement.

@DanRStevens
Copy link
Member Author

I noticed this today, and wanted to add a reference to it. Some of the techniques in that thread may be of use here for detecting a special Write method. I believe they can be adapted to look for a special signature, or to verify that a certain expression would compile.
https://stackoverflow.com/questions/257288/is-it-possible-to-write-a-template-to-check-for-a-functions-existence

@DanRStevens
Copy link
Member Author

I was doing a bit of research today. I took a second look at the link above, and also an additional link that seems to be useful:
https://stackoverflow.com/questions/257288/is-it-possible-to-write-a-template-to-check-for-a-functions-existence
https://stackoverflow.com/questions/23383599/how-to-find-out-if-a-type-has-member-function-with-any-return-type

In both threads, there are examples of the newer trailing return type syntax. It seems to play really nice with feature detection, in terms of deciding up front if a given line of code will compile. I may start to prefer that syntax over placing enable_if in front of the method. The example in the second thread seems much more readable than previous examples.

template <typename C, typename... Args>
struct is_call_possible {
private:
    template<typename T>
    static auto check(int)
        -> decltype( std::declval<T>().operator()(std::declval<Args>()...),
                     // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                     // overload is removed if this expression is ill-formed
           std::true_type() );

    template<typename>
    static std::false_type check(...);
public:
    static constexpr bool value = decltype(check<C>(0))::value;
};

In the check<C>(0) unevaluated call at the end, the 0 is to prefer the first overload check(int) (with return type true_type), as opposed to the second overload check(...) (with return type false_type).

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

No branches or pull requests

2 participants