thestk / rtmidi

A set of C++ classes that provide a common API for realtime MIDI input/output across Linux (ALSA & JACK), Macintosh OS X (CoreMIDI) and Windows (Multimedia)
Other
993 stars 275 forks source link

preprocessor defines shouldn't start with underscore #133

Open umlaeute opened 7 years ago

umlaeute commented 7 years ago

RtMidi uses defines like __UNIX_JACK__.

however, preprocessor defines (or any identifier, really) starting with _ (single underscore) are reserved. preprocessor defines (or any identifier, really) starting with __ (double underscore) are reserved for the compiler implementation.

a much better naming scheme would be to prefix all defines with a "namespace", e.g. RTMIDI_ so you get defines like RTMIDI_USE_UNIX_JACK (or whatever you prefer).

This would also allow to have independent backend selection for RtMidi and RtAudio in a project that includes both.

see also #131

umlaeute commented 7 years ago

i could provide a PR, but I don't know much about the implications of changing those defines (e.g. will it break the API?)

umlaeute commented 7 years ago

I searched for all of the backend-specific classes on https://codesearch.debian.net/ and at least in Debian there does not seem to be any use of things like MidiInJack (or MidiOutCore, or ...).

there's a couple of packages in Debian that use RtMidi (and RtAudio), so that is at least promising.

for the record, the actual search string was something like -path:.*RtMidi.h -path:.*RtMidi.cpp MidiInCore\b (this filters out the verbatim copies of RtMidi in the various packages).

jpcima commented 6 years ago

It disturbs me as well. Maybe RtMidi can adopt a way which accepts either style of definition and deprecate the former, so this would not break the existing programs.

Maybe something like this at the top of the platform specific part.

#ifdef __LINUX_ALSA__
#define RTMIDI_LINUX_ALSA
#pragma message("don't use me I'm deprecated")
#endif
radarsat1 commented 6 years ago

I labelled this C++11 not because it has anything to do with C++11 but because that label for me represents a time when we will integrate changes that might break user programs.

sagamusix commented 1 year ago

Maybe it's time to revive this. Recently a new API was added (Windows UWP), and while I tried to integrate it into my application, I saw that __WINDOWS_UWP__ needs to be defined in order to enable this. Due to the double underscores, this is a reserved identifier and my first assumption was that the compiler / platform SDK would define this preprocessor constant. But no, this actually has to be supplied by the user. The fact that the name of this constant doesn't even contain RTMIDI or similar makes it even more confusing.

My proposition would be, for RtMidi 7, to rename all RtMidi preprocessor flags to something standard-confirming, e.g. RTMIDI_WINDOWS_UWP instead of __WINDOWS_UWP__.

sagamusix commented 1 year ago

Or when using the approach suggested by @jpcima, I guess the change could already be made in a subsequent 6.x.x release, effectively deprecating the existing defines, and then fully removing them in RtMidi 7 if you wish so.