-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use std::optional
rather than sentinel values
#2964
base: master
Are you sure you want to change the base?
Conversation
6e51ef4
to
8a39b5a
Compare
d949d15
to
f3cab9b
Compare
Pull Request Test Coverage Report for Build 9273418738Details
💛 - Coveralls |
f3cab9b
to
738380c
Compare
738380c
to
7b6fdb1
Compare
Rebased on |
7b6fdb1
to
610e348
Compare
610e348
to
20fe172
Compare
I added more |
20fe172
to
7f4107d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a good idea to me!
7f4107d
to
399e7ce
Compare
399e7ce
to
f479ca7
Compare
Found some Android APIs that return |
c79efcc
to
f7e35d8
Compare
f7e35d8
to
fd7f69a
Compare
fd7f69a
to
cfef73a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good, some suggestions
@@ -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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -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()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
if (const auto position = data->stream->tell()) | |
if (const auto position = data->stream->tell(); position.has_value()) |
@@ -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()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (const auto count = data->stream->getSize()) | |
if (const auto count = data->stream->getSize(); count.has_value()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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())
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return position ? 0 : -1; | |
return position.has_value() ? 0 : -1; |
const auto newPosition = AAsset_seek(m_file.get(), static_cast<off_t>(position), SEEK_SET); | ||
if (newPosition < 0) | ||
return std::nullopt; | ||
return newPosition; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, like above
//////////////////////////////////////////////////////////// | ||
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; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use ternaries here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
cfef73a
to
4713bfc
Compare
Rebased on |
ad82b0b
to
51b0492
Compare
Rebased on |
51b0492
to
48e8068
Compare
48e8068
to
0a8fe39
Compare
0a8fe39
to
d6176ce
Compare
Description
sf::InputStream
and derived classes have four functions whose return value operates in the same way,open
,read
,seek
, andtell
. 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 fromgetSize()
. However, the type system allows this. Because both return values are of typestd::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 withstd::optional<std::size_t>
. Usingstd::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
What happens if it does not succeed? In the case that
-1
is returned then that value is underflowed to the maximum value ofstd::size_t
. When divided bysizeof(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
andgetSize
are used without handling the potential error case. It is not valid to incrementoffset
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
In this case I chose to use
.value()
to ensure that the optional has a value or else letstd::bad_optional_access
be thrown. We could instead simply useoperator*
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_cast
s by a count of 19. Removing the need for casts is a good sign thatstd::optional<std::size_t>
is a more natural fit thanstd::int64_t
.