Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Writer class for Boost serialization by moving method implementations from the header to a source file and utilizing explicit template instantiation for specific archive types. Feedback indicates that this approach is a breaking change for users of other Boost archive types, such as xml_oarchive, and suggests using a separate template implementation file instead. Furthermore, the inclusion of a .cpp file within another source file was identified as a violation of common practices, with a recommendation to use a more appropriate file extension like .tcc.
| *ar_ << static_cast<std::uint64_t>(_size); | ||
| return OutputArrayType{}; | ||
| } | ||
| OutputArrayType array_as_root(const size_t _size) const; |
There was a problem hiding this comment.
Moving the implementation of these methods to a .cpp file and using explicit instantiation restricts the Writer class to only the archive types explicitly handled by the library. This is a breaking change for users of other Boost archive types, such as xml_oarchive or custom archives, as they will now encounter linker errors for these non-template methods. To maintain support for arbitrary archives while still improving compile times, consider moving the implementations to a separate header file (e.g., Writer.tcc) and including it at the end of this header, or keep the implementations inline.
| @@ -0,0 +1 @@ | |||
| #include "rfl/boost_serialization/Writer.cpp" | |||
There was a problem hiding this comment.
Including a .cpp file is generally discouraged as it can lead to multiple definition errors if the included file is also compiled as a separate translation unit. It also violates common naming conventions for implementation files intended for inclusion. Consider renaming src/rfl/boost_serialization/Writer.cpp to a more appropriate extension like .tcc or .ipp, or moving the code directly into this file.
|
@ron0studios, yes, somehow the rebase broke that. But there still seems to be something wrong with it, some tests fail. COuld you take a look? |
2 participants
@liuzicheng1987 sorry I think this change got lost during a rebase, but slipped under the radar since all tests still pass without it.