oboe: add ErrorOrValue dual return #63
Conversation
For returning an error code and a frame count, or other value, at the same time.
include/oboe/ErrorOrValue.h
Outdated
| @@ -0,0 +1,58 @@ | |||
| /* | |||
| * Copyright (C) 2016 The Android Open Source Project | |||
include/oboe/ErrorOrValue.h
Outdated
|
|
||
| /** | ||
| * Quick way to check for an error. | ||
| * @return true if an error occurred // TODO does this seem backwards? |
There was a problem hiding this comment.
To me, "backwards" seems more natural:
if (!write(...)) { // handle error }
Thus, operator! should return 'true' if there was an error.
include/oboe/ErrorOrValue.h
Outdated
| , mError(error) {} | ||
|
|
||
| explicit ErrorOrValue(T value) | ||
| : mValue(value < 0 ? 0 : value) |
There was a problem hiding this comment.
That's not generic. What if 'T' is bool, or a string? In fact, it's the responsibility of the function that returns ErrorOrValue to determine the error condition, and use the appropriate type to pass to the constructor. If the constructor that takes 'T' is used, ErrorOrValue should assume that it was a success.
| } | ||
|
|
||
| T value() const { | ||
| return mValue; |
There was a problem hiding this comment.
Should check if there is no error, and emit a log message.
There was a problem hiding this comment.
Might be useful if a programmer claims that "oboe doesn't work," and then from the logs it turns out they are not checking the result, and there was an error. Treble for example has even stronger checks where retrieving the value crashes if the error status hasn't been checked before value retrieval. But that feels a bit too harsh, a simple log should be enough.
There was a problem hiding this comment.
@mnaganov - I do not understand this request. Do you want to log every access to the value?!
There was a problem hiding this comment.
Only if there was an error (mError != OK). Accessing the value if there was an error indicates a problem with client code.
There was a problem hiding this comment.
I see. Good idea. #76
src/aaudio/AudioStreamAAudio.cpp
Outdated
| return mLibLoader->stream_write(mAAudioStream, buffer, numFrames, timeoutNanoseconds); | ||
| int32_t result = mLibLoader->stream_write(mAAudioStream, buffer, | ||
| numFrames, timeoutNanoseconds); | ||
| return ErrorOrValue<int32_t>(result); |
There was a problem hiding this comment.
As I've noted, it's the responsibility of AudioStreamAAudio::write to implement the logic that considers whether the value of result indicates an error or not. ErrorOrValue must not know these details.
Inverted boolean operators. More generic constructors.
| } | ||
|
|
||
| T value() const { | ||
| return mValue; |
There was a problem hiding this comment.
Might be useful if a programmer claims that "oboe doesn't work," and then from the logs it turns out they are not checking the result, and there was an error. Treble for example has even stronger checks where retrieving the value crashes if the error status hasn't been checked before value retrieval. But that feels a bit too harsh, a simple log should be enough.
| @@ -98,23 +98,23 @@ int64_t AudioStreamBuffered::predictNextCallbackTime() { | |||
| // TODO: Consider returning an error_or_value struct instead. | |||
There was a problem hiding this comment.
Nit: Remove this comment now method returns ErrorOrValue
For returning an error code and a frame count, or other value, at the same time.