Skip to content
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

Make RtMidi moveable and non-copyable #238

Closed
wants to merge 3 commits into from

Conversation

jkeller51
Copy link
Contributor

@jkeller51 jkeller51 commented Mar 3, 2021

Make RtMidi, RtMidiIn, and RtMidiOut moveable, but non-copyable.

This was branched off of #237 -- Make RtMidi non-copyable.

Essentially I was trying to make a resizable std::vector<RtMidiIn>, but it didn't work as expected. Resizing std::vector requires copying or moving data when the internal array resizes, but RtMidi's copy was not well-defined and ultimately caused the new RtMidi instances to have a dangling pointer to deleted memory (rtapi_).

Copying an RtMidi doesn't make sense at the moment because it will always destroy its rtapi_ when it destructs, so two RtMidi instances sharing their rtapi_ will always result in an error. For this reason I made it non-copyable.

It was pointed out in #237 that making RtMidi moveable could still allow it to be used with std::vector. It also shouldn't cause any error conditions. So I implemented a move constructor for RtMidi and its derived classes.

Tested and working in Windows.

@jkeller51 jkeller51 changed the title Moveable Make RtMidi moveable and copyable Mar 3, 2021
@jkeller51 jkeller51 changed the title Make RtMidi moveable and copyable Make RtMidi moveable and non-copyable Mar 3, 2021
@jkeller51
Copy link
Contributor Author

^^^ c++11 wasn't mandated by the CMakeLists.txt, and it's not immediately clear to me that it's required, so I included a macro for noexcept that will evaluate to throw() for versions of c++ older than c++11.

@keryell
Copy link

keryell commented Mar 4, 2021

Just curious, what is the motivation to support C++03? 18 years ago...

@jkeller51
Copy link
Contributor Author

It doesn't matter to me, I only use c++11 and up. It just seemed a little presumptuous of me to come in here and introduce a new rule as part of my changes when it could be avoided.

@radarsat1
Copy link
Contributor

I tested this and it works. The main question I have is what happens to the moved object? You still have an RtMidiOut/In object after the move, but it no longer has a valid internal rtapi_ pointer, which leads to segfaults if it is used. Do we need to add checks for every forwarded function? (Maybe in debug build... which is not ideal since forwarded functions are implemented inline in the header.) I realize it's probably up to the programmer to avoid this but is there anything that can be done?

I played around with replacing MidiApi *rtapi_ with std::shared_ptr<MidiApi> rtapi_ and that also solves this problem. Pros, cons?

I also found that replacing it with std::unique_ptr has the effect of forcing this solution rather nicely, so also an attractive possibility.

@radarsat1
Copy link
Contributor

In the meantime this code was merged in #255, where I removed the backwards compatibility with C++98 in favor of just requiring C++11. So closing this, but please add any comments you might have on std::shared_ptr. (Changing it would not affect the API except to allow copy properly.) I suppose although it works, in principle the RtMidi objects represent unique instances of a device so it doesn't really make sense to support copy, although it is just a handle to an internal data structure. The shared_ptr has some small reference counting overhead that is maybe not justified? Although that's probably negligible.

@radarsat1 radarsat1 closed this Aug 11, 2021
@keryell
Copy link

keryell commented Aug 12, 2021

I tested this and it works. The main question I have is what happens to the moved object? You still have an RtMidiOut/In object after the move, but it no longer has a valid internal rtapi_ pointer, which leads to segfaults if it is used. Do we need to add checks for every forwarded function? (Maybe in debug build... which is not ideal since forwarded functions are implemented inline in the header.) I realize it's probably up to the programmer to avoid this but is there anything that can be done?

Exactly.
It is undefined behavior to use a moved-from object in C++.
It is up to the programmer to make sure that the moved-from object is valid enough to be safely destructed later.

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.

None yet

3 participants