Use std::optional rather than sentinel values

[ChrisThrasher]

Description

sf::InputStream and derived classes have four functions whose return value operates in the same way, open, read, seek, and tell. Upon success, they return a non-negative number which means different things for different functions. Upon failure, they return -1. -1 is a sentinel value. This error value is incompatible with other successful return values. It's not valid to, for example, add -1 to a non-negative return value from getSize(). However, the type system allows this. Because both return values are of type std::int64_t it's very easy to ignore a potential -1 return value and instead write code that behaves unexpected in the face of errors.

This PR replaces those std::int64_t return values with std::optional<std::size_t>. Using std::optional forces all callers of these functions to reckon with a potential error. This has major type safety implications since it's no longer possible to silently ignore the possibility of -1 being returned.

As it turns out, there are places where these functions were called without taking into consideration the possibility of failure. For example the following snippet assumes that file.getSize() succeeds.

SFML/examples/vulkan/Vulkan.cpp

Line 934 in 73126c9

std::vector<std::uint32_t> buffer(static_cast<std::size_t>(file.getSize()) / sizeof(std::uint32_t));

What happens if it does not succeed? In the case that -1 is returned then that value is underflowed to the maximum value of std::size_t. When divided by sizeof(std::uint32_t) you end up with a value that is certainly too large to be successfully allocated. Failure to check this return value morphs into an allocation failure which is a confusing user experience.

Here's another example. The return values of tell and getSize are used without handling the potential error case. It is not valid to increment offset by -1. -1 does not have any arithmetic meaning. It is a placeholder value. Subtracting the offset from -1 leads to a number that is even more negative. Yet another nonsense value yet the existing API did nothing to force us to handle this error, highlighting its lack of safety.

SFML/src/SFML/Audio/SoundFileReaderOgg.cpp

Lines 55 to 58 in 73126c9

	offset += stream->tell();
	break;
	case SEEK_END:
	offset = stream->getSize() - offset;

In this case I chose to use .value() to ensure that the optional has a value or else let std::bad_optional_access be thrown. We could instead simply use operator* but then we open ourselves up to undefined behavior in the event that the optional is null which seemed like a worse user experience that an uncaught exception causing a program exit.

In some places we're working with C APIs that still expect -1 to signal error and those circumstances are still handled, albeit handled in a more explicit way that makes it clear that the C APIs handle errors differently than SFML itself.

In performing this refactor I reduced the number of static_casts by a count of 19. Removing the need for casts is a good sign that std::optional<std::size_t> is a more natural fit than std::int64_t.

[coveralls]

Pull Request Test Coverage Report for Build 9373018345

Details

• 77 of 80 (96.25%) changed or added relevant lines in 12 files are covered.
• No unchanged relevant lines lost coverage.
• Overall first build on optional_input_stream at 55.821%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/SFML/Audio/SoundFileReaderFlac.cpp	9	10	90.0%
src/SFML/System/FileInputStream.cpp	14	16	87.5%

Totals
Change from base Build 9373011591:	55.8%
Covered Lines:	11577
Relevant Lines:	19662

---

💛 - Coveralls

[ChrisThrasher]

Rebased on master. No changes made.

[ChrisThrasher]

I added more sf::MemoryInputStream tests to better cover edge case behavior.

[vittorioromeo]

Seems like a good idea to me!

[ChrisThrasher]

Found some Android APIs that return -1 on failure and thus need some extra code to check for that case to return std::nullopt.

[vittorioromeo]

Generally looks good, some suggestions

[ChrisThrasher]

With respect to the use of has_value() I've gone back and forth while writing this PR. Sometimes I like explicitly saying this is an optional but other times I prefer the elegance of letting the type system quietly handle things even if the reader doesn't necessarily know what's going on. I'm currently erring on the side of preferring we write less code since doing so doesn't reduce type safety whatsoever.

With respect to the use of ternaries, I don't have a strong opinion. In the past we haven't made a point to maximize the use of ternaries. If the code was already written to use ternaries I wouldn't replace them with if statements. I don't really see either as particularly superior to the other so I just kept using whatever control flow already existed prior to this PR to reduce how much code I changed.

In both cases if the rest of the team agrees in one direction or the other I'll go along with what they prefer.

[ChrisThrasher]

Rebased on master and fixed conflicts

[ChrisThrasher]

Rebased on master and fixed conflicts

[vittorioromeo]

See inline comments

[ChrisThrasher]

Replaced const auto foo = returns_optional(); with const std::optional foo = returns_options(); and rebased on master.

[vittorioromeo]

LGTM, thanks for making the readability related changes!

[eXpl0it3r]

In performing this refactor I reduced the number of static_casts by a count of 19. Removing the need for casts is a good sign that std::optional<std::size_t> is a more natural fit than std::int64_t.

We'll have to look at this again. size_t is differently sized than int64_t and the stream APIs (fseek, ftell) use long, while write and read use size_t.
int64_t might not be the right size, given that long is at least 32bits.
size_t however is only guaranteed to be at least 16bits. I guess in most cases we can expect 32bits, but it's not guaranteed.