Bugfix: Clamp Note.noteNumber to MIDI range (0-127) to prevent crashes #57
Conversation
- Add early bounds checking for octave values outside -2 to 8 range. Fixes infinite loop for octaves below -2 - Add final bounds checking to ensure note values are within MIDI range. Prevents crash on cast to Int8 for values > 127 - Add test coverage
| let gSharp8 = Note(.G, accidental: .sharp, octave: 8) | ||
| XCTAssertEqual(gSharp8.noteNumber, 127) | ||
|
|
||
| let c9 = Note(.C, octave: 9) |
There was a problem hiding this comment.
I'm following what appears to be a well established convention in the tests for identifiers referring to notes
| let cFlatMinus3 = Note(.C, accidental: .flat, octave: -3) | ||
| XCTAssertEqual(cFlatMinus3.noteNumber, 0) | ||
|
|
||
| let a8 = Note(.A, octave: 8) |
There was a problem hiding this comment.
I'm following what appears to be a well established convention in the tests for identifiers referring to notes
|
|
||
| let gSharp8 = Note(.G, accidental: .sharp, octave: 8) | ||
| XCTAssertEqual(gSharp8.noteNumber, 127) | ||
|
|
There was a problem hiding this comment.
I'm following the spacing convention established in other tests in this file, where assertion statements
are grouped with their supporting setup statements using blank lines. This convention
helps clarify the logical grouping and makes the test structure more readable.
|
|
||
| let a8 = Note(.A, octave: 8) | ||
| XCTAssertEqual(a8.noteNumber, 127) | ||
|
|
There was a problem hiding this comment.
I'm following the spacing convention established in other tests in this file, where assertion statements
are grouped with their supporting setup statements using blank lines. This convention
helps clarify the logical grouping and makes the test structure more readable.
|
|
||
| let cFlatMinus3 = Note(.C, accidental: .flat, octave: -3) | ||
| XCTAssertEqual(cFlatMinus3.noteNumber, 0) | ||
|
|
There was a problem hiding this comment.
I'm following the spacing convention established in other tests in this file, where assertion statements
are grouped with their supporting setup statements using blank lines. This convention
helps clarify the logical grouping and makes the test structure more readable.
|
|
||
| let bMinus3 = Note(.B, octave: -3) | ||
| XCTAssertEqual(bMinus3.noteNumber, 0) | ||
|
|
There was a problem hiding this comment.
I'm following the spacing convention established in other tests in this file, where assertion statements
are grouped with their supporting setup statements using blank lines. This convention
helps clarify the logical grouping and makes the test structure more readable.
2 participants
It is possible to create instances of the Note struct that represent values outside of the 0-127 MIDI note range. In such a case, accessing the Note.noteNumber calculated property may result in either an infinite loop or a crash. If the octave is set to less than -2, the following gets stuck in an infinite loop:
Also, if the final calculated note value is greater than 127, it will cause a crash when being cast to Int8 prior to being returned.
The fix: