Cleanup stream read method guards #288
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Matches style used for stream write code.
Guard against the container itself being trivial to avoid ambiguous calls. This excludes `std::array`, which is served by the other method taking trivial types. A trivial container of trivial types is itself trivial. Note: Technically, this should guard for types with `.data()` and `.size()` members. Such members imply a contiguous container.
These methods are basically identical, and may be combined into a single method at some point.
It could potentially be collapsed with the method above, but needs C++17 flags set to allow that. Reference: https://en.cppreference.com/w/cpp/string/basic_string/data
This can potentially handle the size prefix part of any container read, including containers we don't yet support serialization for. Currently we simply delegate for the details of reading the actual container contents.
This is now served by the more general size prefixed container read case.
Member
|
If PR #290 is merged, this branch can use C++17 features for MSVC builds. |
Member
Author
|
I just took another look at this. It should be fine to merge now. Once we enable C++17 on all systems we could then remove the one largely redundant method. If we want to do the method removal as part of this PR, then we should wait until the other changes are merged first. |
Member
|
Unless we plan to update to requiring C++17 now on all Linux compilers, I would prefer merging and writing up an issue for later work. Better to close this branch out instead of letting it grow stale over time. We could create a C++17 tag for issues as a reminder on which ones are waiting on C++17 as standard to complete. |
Member
Author
|
Ok, very good suggestion. I wrote up an issue. I'll merge this now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the read update corresponding to PR #287. Tagging Issue #278 as related. Tagging PR #281 as related for simplifying the template parameters.
There was one slight difference to the write case. I didn't collapse one of the string read methods into the generalized container form, since the
string.data()method didn't have a non-const overload until C++17. Reference:https://en.cppreference.com/w/cpp/string/basic_string/data
The current Linux flags are set for C++14. I'm uncertain if there are blocking issues with the Windows project, particularly regarding the Google Test unit test project. If it's fine to update to C++17, then we can collapse those two methods into one.