Skip to content
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

ResultWithValue: Doc updates, version bump and code changes #109

Merged
merged 5 commits into from Jun 14, 2018

Conversation

dturner
Copy link
Collaborator

@dturner dturner commented Jun 13, 2018

v0.11 ResultWithValue - BREAKING CHANGE!

This change (and this one) add a new object ResultWithValue and change some of the AudioStream method signatures in the Oboe library.

Why make this change?

Some of the AudioStream methods would return a positive (or zero) value if they were successful and a negative value if not. For example, consider the following code which attempts to set a stream's buffer size:

int32_t result = stream->setBufferSizeInFrames(newBufferSize)
if (result < 0){
    LOGD("Error setting buffer size: %s", convertToText(static_cast<Result>(result));
} else {
    LOGD("New buffer size is: %d", result);
}

The setBufferSizeInFrames method is actually returning two pieces of information:

  1. whether the call succeeded or failed
  2. the new buffer size if the call succeeded

The method breaks the single responsibility principle which can lead to inelegant and confusing code. Also, casting an int32_t to a Result is a bug waiting to happen!

What's new then?

To solve this problem we added a new object ResultWithValue which encapsulates the 2 pieces of information above.

You can check whether an operation was successful by comparing the ResultWithValue with a Result (e.g. if (result != Result::OK) { // error }, or by calling ResultWithValue::error().

The value can be retrieved by calling ResultWithValue::value().

The above code can now be made more readable:

auto result = stream->setBufferSizeInFrames(newBufferSize)
if (result != Result::OK){
    LOGD("Error setting buffer size: %s", convertToText(result.error());
} else {
    LOGD("New buffer size is: %d", result.value());
}

Which methods are affected?

The following methods in AudioStream have updated method signatures:

ResultWithValue<int32_t> setBufferSizeInFrames(int32_t requestedFrames)
ResultWithValue<int32_t> getXRunCount()
ResultWithValue<double> calculateLatencyMillis()
ResultWithValue<int32_t> write(const void *buffer,
                         int32_t numFrames,
                         int64_t timeoutNanoseconds)
virtual ResultWithValue<int32_t> read(void *buffer,
                        int32_t numFrames,
                        int64_t timeoutNanoseconds)

If you are using any of the above methods you'll need to update your code in line with the example above.

FullGuide.md Outdated
return DataCallbackResult::Continue;
}
auto result = stream2.read(audioData, numFrames, timeout);
if (result == Result::OK){
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment here why it's possible first to compare result with an value of Result and then with a number, and what does it mean. Otherwise might look confusing for a person not very familiar with type coercion operator overloading.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, done.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this update. Have you uploaded it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't hit save in my text editor 🤦‍♂️ . Fixed now.

@@ -1,5 +1,5 @@
/*
* Copyright 2017 The Android Open Source Project
* Copyright 2018 The Android Open Source Project
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that you don't have to update the year when editing existing files. New files need to receive the current year when added.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted, thanks for the info.

}

// When state is Active attempt to change the buffer size if the number of xRuns has increased.
if (mState == State::Active) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the previous version the code was setting state to Active and then breaking out of case. In this edition the behavior changes. Is it intentional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good spot. I've amended to keep in line with the old behaviour.

With hindsight it might have been better to let @philburk refactor this code. It probably wants splitting into smaller methods.

@dturner dturner merged commit 279d59c into master Jun 14, 2018
@dturner dturner deleted the resultwithvalue-part2 branch June 14, 2018 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants