Use `std::optional` rather than sentinel values #2964

ChrisThrasher · 2024-04-29T19:55:50Z

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.

src/SFML/Audio/SoundFileReaderWav.cpp

coveralls · 2024-04-30T00:40:24Z

Pull Request Test Coverage Report for Build 9150171440

Details

77 of 80 (96.25%) changed or added relevant lines in 12 files are covered.
2 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.05%) to 55.611%

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%

Files with Coverage Reduction	New Missed Lines	%
src/SFML/Audio/SoundFileReaderWav.cpp	1	75.0%
src/SFML/Graphics/Font.cpp	1	76.52%

Totals
Change from base Build 9150167137:	0.05%
Covered Lines:	11476
Relevant Lines:	19543

💛 - Coveralls

src/SFML/System/MemoryInputStream.cpp

src/SFML/Graphics/Shader.cpp

src/SFML/System/MemoryInputStream.cpp

ChrisThrasher · 2024-05-02T03:05:21Z

Rebased on master. No changes made.

src/SFML/System/FileInputStream.cpp

ChrisThrasher · 2024-05-02T17:48:22Z

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

vittorioromeo

Seems like a good idea to me!

examples/vulkan/Vulkan.cpp

include/SFML/System/FileInputStream.hpp

ChrisThrasher · 2024-05-03T04:44:37Z

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

vittorioromeo · 2024-05-07T16:07:51Z

src/SFML/Audio/SoundFileReaderFlac.cpp

@@ -44,15 +44,17 @@ FLAC__StreamDecoderReadStatus streamRead(const FLAC__StreamDecoder*, FLAC__byte
 {
 auto* data = static_cast<sf::priv::SoundFileReaderFlac::ClientData*>(clientData);

- const std::int64_t count = data->stream->read(buffer, static_cast<std::int64_t>(*bytes));
- if (count > 0)
+ if (const auto count = data->stream->read(buffer, *bytes))


Suggested change

if (const auto count = data->stream->read(buffer, *bytes))

if (const auto count = data->stream->read(buffer, *bytes); count.has_value())

Nitpick, but this suggestion makes it obvious that count is an optional in this context

vittorioromeo · 2024-05-07T16:08:22Z

src/SFML/Audio/SoundFileReaderFlac.cpp

@@ -75,10 +76,9 @@ FLAC__StreamDecoderTellStatus streamTell(const FLAC__StreamDecoder*, FLAC__uint6
 {
 auto* data = static_cast<sf::priv::SoundFileReaderFlac::ClientData*>(clientData);

- const std::int64_t position = data->stream->tell();
- if (position >= 0)
+ if (const auto position = data->stream->tell())


Ditto

Suggested change

if (const auto position = data->stream->tell())

if (const auto position = data->stream->tell(); position.has_value())

vittorioromeo · 2024-05-07T16:08:30Z

src/SFML/Audio/SoundFileReaderFlac.cpp

@@ -91,10 +91,9 @@ FLAC__StreamDecoderLengthStatus streamLength(const FLAC__StreamDecoder*, FLAC__u
 {
 auto* data = static_cast<sf::priv::SoundFileReaderFlac::ClientData*>(clientData);

- const std::int64_t count = data->stream->getSize();
- if (count >= 0)
+ if (const auto count = data->stream->getSize())


Suggested change

if (const auto count = data->stream->getSize())

if (const auto count = data->stream->getSize(); count.has_value())

@ChrisThrasher: The reason why I suggested this change (and similar ones) is that if I am just reading this line of code in isolation I will assume that getSize() returns a number and that count is just a number. Nothing in the name of the variable nor the name of the function suggests optionality.

I would rather read

if (const auto count = data->stream->getSize(); count.has_value())

or

if (const std::optional count = data->stream->getSize())

or

if (const std::optional<std::size_t> count = data->stream->getSize())

src/SFML/Audio/SoundFileReaderMp3.cpp

vittorioromeo · 2024-05-07T16:10:03Z

src/SFML/Audio/SoundFileReaderMp3.cpp

- return position < 0 ? -1 : 0;
+ auto* stream = static_cast<sf::InputStream*>(data);
+ const auto position = stream->seek(static_cast<std::size_t>(offset));
+ return position ? 0 : -1;


Suggested change

return position ? 0 : -1;

return position.has_value() ? 0 : -1;

vittorioromeo · 2024-05-07T16:17:51Z

src/SFML/System/Android/ResourceStream.cpp

+ const auto newPosition = AAsset_seek(m_file.get(), static_cast<off_t>(position), SEEK_SET);
+ if (newPosition < 0)
+ return std::nullopt;
+ return newPosition;


Ditto, like above

src/SFML/System/Android/ResourceStream.cpp

src/SFML/System/MemoryInputStream.cpp

vittorioromeo · 2024-05-07T16:21:46Z

src/SFML/System/MemoryInputStream.cpp

 ////////////////////////////////////////////////////////////
-std::int64_t MemoryInputStream::tell()
+std::optional<std::size_t> MemoryInputStream::tell()
 {
 if (!m_data)
- return -1;
+ return std::nullopt;

 return m_offset;
 }


 ////////////////////////////////////////////////////////////
-std::int64_t MemoryInputStream::getSize()
+std::optional<std::size_t> MemoryInputStream::getSize()
 {
 if (!m_data)
- return -1;
+ return std::nullopt;

 return m_size;
 }


I'd use ternaries here

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.

src/SFML/System/Android/ResourceStream.cpp

src/SFML/System/MemoryInputStream.cpp

ChrisThrasher · 2024-05-14T20:08:12Z

Rebased on master and fixed conflicts

ChrisThrasher · 2024-05-18T04:01:06Z

Rebased on master and fixed conflicts

ChrisThrasher added the feature label Apr 29, 2024

ChrisThrasher added this to the 3.0 milestone Apr 29, 2024

ChrisThrasher requested review from eXpl0it3r and binary1248 April 29, 2024 19:55

ChrisThrasher force-pushed the optional_input_stream branch from 6e51ef4 to 8a39b5a Compare April 29, 2024 20:06

kimci86 reviewed Apr 29, 2024

View reviewed changes

src/SFML/Audio/SoundFileReaderWav.cpp Outdated Show resolved Hide resolved

ChrisThrasher force-pushed the optional_input_stream branch 2 times, most recently from d949d15 to f3cab9b Compare April 30, 2024 00:04

ChrisThrasher commented Apr 30, 2024

View reviewed changes

src/SFML/System/MemoryInputStream.cpp Outdated Show resolved Hide resolved

ChrisThrasher force-pushed the optional_input_stream branch from f3cab9b to 738380c Compare April 30, 2024 03:28

kimci86 reviewed Apr 30, 2024

View reviewed changes

src/SFML/Graphics/Shader.cpp Outdated Show resolved Hide resolved

src/SFML/System/MemoryInputStream.cpp Outdated Show resolved Hide resolved

ChrisThrasher force-pushed the optional_input_stream branch from 738380c to 7b6fdb1 Compare May 2, 2024 03:05

ChrisThrasher force-pushed the optional_input_stream branch from 7b6fdb1 to 610e348 Compare May 2, 2024 03:37

ChrisThrasher commented May 2, 2024

View reviewed changes

src/SFML/System/FileInputStream.cpp Outdated Show resolved Hide resolved

ChrisThrasher force-pushed the optional_input_stream branch from 610e348 to 20fe172 Compare May 2, 2024 17:47

ChrisThrasher force-pushed the optional_input_stream branch from 20fe172 to 7f4107d Compare May 2, 2024 17:49

vittorioromeo reviewed May 2, 2024

View reviewed changes

examples/vulkan/Vulkan.cpp Outdated Show resolved Hide resolved

examples/vulkan/Vulkan.cpp Outdated Show resolved Hide resolved

include/SFML/System/FileInputStream.hpp Outdated Show resolved Hide resolved

ChrisThrasher force-pushed the optional_input_stream branch from 7f4107d to 399e7ce Compare May 2, 2024 18:02

ChrisThrasher mentioned this pull request May 2, 2024

Event::operator bool #2968

Merged

5 tasks

ChrisThrasher force-pushed the optional_input_stream branch from 399e7ce to f479ca7 Compare May 3, 2024 04:44

ChrisThrasher force-pushed the optional_input_stream branch 4 times, most recently from c79efcc to f7e35d8 Compare May 4, 2024 17:37

ChrisThrasher requested a review from vittorioromeo May 4, 2024 17:42

ChrisThrasher force-pushed the optional_input_stream branch from f7e35d8 to fd7f69a Compare May 6, 2024 03:23

ChrisThrasher force-pushed the optional_input_stream branch from fd7f69a to cfef73a Compare May 7, 2024 15:21

vittorioromeo reviewed May 7, 2024

View reviewed changes

ChrisThrasher commented May 7, 2024

View reviewed changes

src/SFML/System/Android/ResourceStream.cpp Show resolved Hide resolved

src/SFML/System/MemoryInputStream.cpp Show resolved Hide resolved

ChrisThrasher force-pushed the optional_input_stream branch from cfef73a to 4713bfc Compare May 14, 2024 20:08

ChrisThrasher force-pushed the optional_input_stream branch 2 times, most recently from ad82b0b to 51b0492 Compare May 18, 2024 04:00

Use std::optional rather than sentinel values

48e8068

ChrisThrasher force-pushed the optional_input_stream branch from 51b0492 to 48e8068 Compare May 19, 2024 20:02

ChrisThrasher requested review from vittorioromeo and kimci86 May 19, 2024 20:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `std::optional` rather than sentinel values #2964

Use `std::optional` rather than sentinel values #2964

ChrisThrasher commented Apr 29, 2024 •

edited

coveralls commented Apr 30, 2024 •

edited

ChrisThrasher commented May 2, 2024

ChrisThrasher commented May 2, 2024

vittorioromeo left a comment

ChrisThrasher commented May 3, 2024

vittorioromeo left a comment

vittorioromeo May 7, 2024

vittorioromeo May 7, 2024

vittorioromeo May 7, 2024

vittorioromeo May 14, 2024

vittorioromeo May 7, 2024

vittorioromeo May 7, 2024

vittorioromeo May 7, 2024

ChrisThrasher left a comment •

edited

ChrisThrasher commented May 14, 2024

ChrisThrasher commented May 18, 2024 •

edited

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

	if (const auto count = data->stream->read(buffer, *bytes))
	if (const auto count = data->stream->read(buffer, *bytes); count.has_value())

	if (const auto position = data->stream->tell())
	if (const auto position = data->stream->tell(); position.has_value())

	if (const auto count = data->stream->getSize())
	if (const auto count = data->stream->getSize(); count.has_value())

	return position ? 0 : -1;
	return position.has_value() ? 0 : -1;

Use std::optional rather than sentinel values #2964

Are you sure you want to change the base?

Use std::optional rather than sentinel values #2964

Conversation

ChrisThrasher commented Apr 29, 2024 • edited

Description

coveralls commented Apr 30, 2024 • edited

Pull Request Test Coverage Report for Build 9150171440

Details

💛 - Coveralls

ChrisThrasher commented May 2, 2024

ChrisThrasher commented May 2, 2024

vittorioromeo left a comment

Choose a reason for hiding this comment

ChrisThrasher commented May 3, 2024

vittorioromeo left a comment

Choose a reason for hiding this comment

vittorioromeo May 7, 2024

Choose a reason for hiding this comment

vittorioromeo May 7, 2024

Choose a reason for hiding this comment

vittorioromeo May 7, 2024

Choose a reason for hiding this comment

vittorioromeo May 14, 2024

Choose a reason for hiding this comment

vittorioromeo May 7, 2024

Choose a reason for hiding this comment

vittorioromeo May 7, 2024

Choose a reason for hiding this comment

vittorioromeo May 7, 2024

Choose a reason for hiding this comment

ChrisThrasher left a comment • edited

Choose a reason for hiding this comment

ChrisThrasher commented May 14, 2024

ChrisThrasher commented May 18, 2024 • edited

Use `std::optional` rather than sentinel values #2964

Use `std::optional` rather than sentinel values #2964

ChrisThrasher commented Apr 29, 2024 •

edited

coveralls commented Apr 30, 2024 •

edited

ChrisThrasher left a comment •

edited

ChrisThrasher commented May 18, 2024 •

edited