MidiBufferIterator is broken

First of all, it’s missing an operator->() so you can’t do iter->getMessage().

Then, MidiBufferIterator::reference isn’t actually a reference but a value. operator*() is returning a temporary object by value. This will break when you use it with STL algorithms. For example, if you copy an iterator and dereference that, it needs to be pointing to the same object, but it doesn’t. You need to return a reference to an object that actually lives inside the MidiBuffer from which the iterator originated. Otherwise this isn’t STL compatible and breaks all kinds of iterator invalidation guarantees. You might get away with using it in a range-based for loop but you can’t do much else with it.

For example, if I want to see whether two MidiBuffers contain the same messages, I would like to use std::equal and give it a predicate, but instead I need to roll my own loop which is more fiddly and error-prone.

I was literally doing this yesterday, and it was a fair bit of code. Being able to use STL for that would be very nice indeed.

I gave this a lot of thought. The difficulty is that MidiBuffer doesn’t directly store MidiMessages, it stores a buffer raw data which gets converted to messages on-demand. Therefore, none of the standard iterator concepts quite fit.

I ended up looking at how the Boost TransformIterator is implemented. There, the reference member has the same type as the result of invoking a unary function on an element of the iterated range. In the case of the MidiBufferIterator, our ‘unary function’ is converting a raw pointer into the underlying MidiBuffer storage into a MidiMessageMetadata instance, so our reference type is MidiMessageMetadata. The TransformIterator also doesn’t have an operator-> when it models a Readable iterator, as far as I can tell, and neither do we, because we don’t want to return a pointer to a temporary.

This kind of iterator is still useful though. It’s definitely an improvement over the old iterator (range-for works now!) and most standard algorithms will work too. For example, in the case where you want to check for equality between two buffers:

bool areEqual(const MidiBuffer &a, const MidiBuffer &b) {
  return std::equal(a.begin(), a.end(), b.begin(), b.end(),
                    [](const auto &x, const auto &y) {
                      return x.samplePosition == y.samplePosition &&
                             std::equal(x.data, x.data + x.numBytes, y.data,
                                        y.data + y.numBytes);
                    });
}
1 Like

For what it’s worth, I expect that it will be possible for the MidiBufferIterator to conform to the C++20 indirectly_readable concept, even though it doesn’t quite fit into the C++17 iterator model.

The Boost iterator library docs are also worth reading:

The iterator categories defined in C++98 are extremely limiting because they bind together two orthogonal concepts: traversal and element access. For example, because a random access iterator is required to return a reference (and not a proxy) when dereferenced, it is impossible to capture the capabilities of vector<bool>::iterator using the C++98 categories.

So it seems even some standard library components are unable to conform to the legacy iterator concepts…

Yeah this is exactly the vector<bool> problem you run into.

MidiBuffer doesn’t directly store MidiMessages, it stores a buffer raw data which gets converted to messages on-demand.

This means that MidiBuffer isn’t a container. Just the way vector<bool> isn’t a container.

I think in C++20 it will be better to treat such cases by not providing old-style iterators at all (because it doesn’t quite work) but instead to treat this as a view instead of a container (because this is what it is) and provide a std::ranges:: style iterator + sentinel pair instead.