Fix compiler warnings and the bugs they point to #8
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
added 7 commits
February 22, 2023 12:40
Since std::deque elements are not stored contiguously but rather as a sequence of individually allocated fixed-size arrays, `Bwrite(&rhs[0], rhs.size() * sizeof(T))` for `std::deque<T> rhs` will read garbage for sufficiently large `rhs.size()`.
Note that the code behaviour after this patch is different. Current implementation stops serialization as soon as an error is detected while previous code would would attempt to serialize every element of a container before reporting failure.
The function ```c++
void f(BinaryStream& bs, const std::vector<int>& v) {
bs << v;
}
```
failed to compile because
`template <typename T> bool BinaryStream::operator<<(const& T)`
was used instead of
`template <typename T> bool BinaryStream::operator<<(std::vector<T>&)`.
In this patch, BinaryStream API was redesigned, duplicating code was
removed and `BinaryStream::operator<<` was made less fragile when
working with const references. The changes are fully backwards
compatible.
Note that normally there is no need for serialization to mutate the
object, so `BinaryStream::operator<<` should be working with const
references only. However, `SerialisableObject::SerialiseWrapper` is not
a constant method because it does both serialization and
deserialization. A proper design would be to separate this logic to two
methods. This is to be considered in the future. For now, since
instances of class SerialisableObject can appear in vectors and other
containers, we have to require non-const references for the
corresponding specializations of `BinaryStream::opreator<<`.
Use `#if 0 ... #endif` instead.
Otherwise deleting pointers to derived classes might cause memory leaks.
…0] instead of rhs.data() In C++ before C++11, meddling with a string using the pointer returned by `string.data()`, `string.c_str()` or `&string[0]` are undefined behaviour. `std::string` was not required storing its contents in a continuous buffer, and GCC implemented copy-on-write strings. Nevertheless, this code apparently worked in the environments it was used, because GCC does store string contents in a continuous buffer, and calling `resize()` before deserializing probably helped with copy-on-writing. In newer standards of C++, this is no longer an undefined behaviour.
|
check if still relavent |
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.
The biggest change here is the redesign of BinaryStream API. One possibly backwards incompatible change is that
BinaryStream::operator<<andBinaryStream::operator>>now short-circuit on failure. Previously when (de-)serializing a container, e.g., std::vector, the operator would attempt to (de-)serialize all its items, and then check for the error. Now it stops and returns false as soon as an error occurs. In both cases if the container was partially (de-)serialized, the stream is left in inconsistent state, so nobody should have relied on this behaviour.Another change is that when serializing a container, BinaryStream now checks its mode (READ, NEW or APPEND) only once. I assume that the mode cannot change during serialization.
Also,
BinaryStream::operator<<emitted compile errors when being passed a const reference to, e.g.,std::vector<int>. I cleaned up the template magic to make it work, see the usage ofis_base_ofandenable_if.While working on BinaryStream API, I noticed that std::deque are serialized in a way that would only work for a small std::deque. The size is implementation-dependendant, and apperently for gcc it is 512 bytes. I fixed that.
Other changes address compiler warnings.