Skip to content

Added missing break. #3673

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

Merged
merged 1 commit into from
Aug 18, 2015
Merged

Added missing break. #3673

merged 1 commit into from
Aug 18, 2015

Conversation

onovy
Copy link
Contributor

@onovy onovy commented Aug 14, 2015

No description provided.

@matthijskooijman
Copy link
Collaborator

The break change you made looks good. AFAICS it wasn't broken without it, it just continued to iterate over a loop when the only valid result in the array was already found, so with the break it should be slightly more efficient.

I don't think the comment change you made is correct, though. The comment "leave timer 0 for last" applies to the original array contents, where multiple timers were available for tone playback. All but 1 timer have been commented out, so there isn't really any leaving to last, so the comment is confusing, but I don't think your change actually improves this.

Could you change your PR to remove the comment change? You can git commit --amend to change your commit locally and then force push it to the same branch to update this PR. While you're at it, perhaps you could improve the commit message a bit too. The trailing dot in the first summary line should be removed and it might be good to mention that this change applies to the Tone library.

Thanks for your contribution!

@onovy onovy force-pushed the toneMinor branch 2 times, most recently from 3e168bb to bf2ef13 Compare August 15, 2015 19:11
@onovy
Copy link
Contributor Author

onovy commented Aug 15, 2015

all done, thanks.

@matthijskooijman
Copy link
Collaborator

Looks good to merge to me.

@cmaglie cmaglie merged commit bf2ef13 into arduino:master Aug 18, 2015
@cmaglie cmaglie added this to the Release 1.6.6 milestone Aug 18, 2015
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.

3 participants