Ensure zero-cost check conditions in Stream

[DanRStevens]

Currently the Stream code contains checks such as:

if (vectorSize < 0) {
  throw std::runtime_error("Vector's size may not be a negative number");
}

This is only possible if the data type of vectorSize is a signed number. In many cases, users of the library will be specifying an unsigned type. For unsigned types, we should try to ensure zero runtime cost for this check.

One possibility, with C++17, is to add a constexpr-if guard around the check:

if constexpr(std::is_signed<SizeType>::value) {
  // ...
}

Currently the project is set to use C++14. Though, the use of the Filesystem API in XFile is technically already C++17. If we are to use constexpr-if, we should change the project settings to use C++17 to avoid warnings over use of constexpr-if.

Caveat: It's unknown if adding these constexpr-if guards is needed to ensure zero runtime cost. The compiler may already be smart enough to realize the existing checks are always false for certain data types. This would be compiler specific though, so we may need to check the output from multiple compilers.

---

One way to test this issue, is to check the assembly output for a small test file:

#include "src/Stream/Reader.h"
#include <vector>

void f(Stream::Reader& r, std::vector<int>& v) {
  r.Read<unsigned int>(v);
}

This file should avoid generating uninteresting code, such as construction of either a Reader or a vector, and instead focus on the call to the Read method, and what that template method expands to. Without optimization though, it still generates quite a bit of code.

Optimized:

clang++ Test.cpp -S -O2

Unoptimized:

clang++ Test.cpp -S

[Brett208]

We have standardized both Linux and Windows version to C++17 at this point, so this effort can move forward.

[DanRStevens]

Ok, I'll add in constexpr checks for signed values before testing against less than 0.

---

Note to future self:

I was also hoping to add constexpr checks before testing overflow of the container size. It might test std::numeric_limits<SizeType>::max to see if the read-in size is guaranteed to be less than the max size the container supports. Example: reader.Read<uint8_t>(vector), which should never overflow the size that a vector supports.

Unfortunately, I don't think testing against potential overflow is really possible. For one, the max_size() property is not constexpr for all containers. It is constexpr for std::array, but not for std::vector: https://en.cppreference.com/w/cpp/container/array/max_size https://en.cppreference.com/w/cpp/container/vector/max_size

Further, approximations don't work, since you need the minimum supported size of a container to test against the max read-in size. It's not possible to approximated max_size by using std::numeric_limits<decltype(container.max_size())>::max as that goes the wrong direction, giving a size too big, rather than a minimum guaranteed supported size.

Perhaps worse is there is no real minimum supported size, since the system could always be out of memory, so even a read of 1 element might be too much. However, that would not have failed the max_size() test, but rather the later container.resize() call, which would still throw an exception, just with perhaps less detailed of an error message.

It's tempting to work in something for containers with a constexpr max_size(), such as std::array, though the method in question wouldn't be used for std::array, as those have a fixed size, and so don't need a length prefixed size to be read in, which is the code that contains these checks.

In short, it only seems sensible to static bounds check on one end for negative signed values. The other side will (and must) rely on runtime checks. Further, those checks will be correct, even if the read-in size is much larger than what the container supports, due to the integral promotion rules that would be applied to the check.

[Brett208]

A couple of days ago I tested trying to use constexpr to check that the max-size was not exceeded and it failed to compile, likely due to what you reported here.