MidiBuffer clear method bug?

General JUCE discussion
1

Hi guys,

Actually I don’t want to state it’s really a bug (since that would mean we have been loosing some MidiMessages for years:) ), but I cannot explain it to myself, maybe someone could help and “proof” why it works as expected?

I’m especially worried about this part: data.removeRange ((int) (start - data.begin()), (int) (end - data.begin()));

This is because the removeRange takes number of element to be removed as its second parameter (and not the end/last element index to be removed) as stated in the docs etc.:

void removeRange (int startIndex, int numberToRemove)

Now in the below call in the MidiBuffer clear method the second param is (int) (end - data.begin(), which means it will contain number of elements from the beginning of data array (while in my opinion it should be number of element from the start pointer).

So shouldn’t it be data.removeRange ((int) (start - data.begin()), (int) (end - begin)); instead? Otherwise in some cases it may simply remove more elements then asked for (if present). Potentially it could even break the whole MidiBuffer (because of the way how MidiMessages are aligned in there as series of uint8 blocks because of different sizes of MidiMessages).

void MidiBuffer::clear (int startSample, int numSamples)

{

auto start = MidiBufferHelpers::findEventAfter (data.begin(), data.end(), startSample - 1);

auto end = MidiBufferHelpers::findEventAfter (start, data.end(), startSample + numSamples - 1);

data.removeRange ((int) (start - data.begin()), (int) (end - data.begin()));

}
2 Likes
2

Somewhat worryingly I think this is a genuine bug that has somehow flown under the radar for the past 10+ years! I’ve pushed a fix and a few tests here. Thanks for flagging this!

2 Likes
3

I am curious about that fix. That change has this line:

 data.removeRange ((int) (start - data.begin()), (int) (end - start));

Is that because “end” really means “1 past the end of the items to remove”? If not, then that should be “end + 1 - start”, not “end - start”. (Personally, I always go to the trouble of naming such variables onePastEnd or endPlusOne, to be certain I know where it is pointing.)

4

Yes, end points to the index of the first event to keep, so it’s the end of a half-open range. The stdlib also uses end for iterators that point one-past-the-end of a range.

2 Likes