-
Notifications
You must be signed in to change notification settings - Fork 77
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
Fix: VAB parsing 1 too many instruments #283
Conversation
Technically this can overflow. I'm not sure if it has practical effects, but this fixes it.
The previous example would parse N+1 instruments, when only N exist.
Please take a look. When the number of instruments was 1, the code would parse first the 0 offset, then the 1 offset. But instrument 1 doesn't exist. This probably wouldn't cause too many problems, but if there was 512 instruments used, this would seemingly start doing some wacky things. |
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.
Thank you for your contribution! The original code was strange and confusing to me, and I think your changes are reasonable. I made a few suggestions, albeit trivial ones.
Co-authored-by: loveemu <loveemu@gmail.com>
Co-authored-by: loveemu <loveemu@gmail.com>
@sykhro I have approved the pull request. There are several merge strategies to choose from, do you have any recommendations? |
I'm okay with anything but squashing. Perhaps we can go for rebase & merge, up to you. Whatever we end up choosing it should probably become the default from now on |
Fix: VAB parsing 1 too many instruments
Edit: I changed this from a nit, to an actual bug fix.