Macro cleanup

[irrenhaus3]

As a preamble for larger refactoring tasks, I'm working on replacing C Preprocessor macros throughout the codebase with type-safe C++ constructs, since they are much easier to reason about and the preprocessor is unaware of language semantics.
The main motivation here was the macro gui, which prevents us from creating a namespace gui as currently being discussed in #6086. The purpose of this PR is to replace as many macros as possible with C++ constructs.

[LmmsBot]

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

macOS

• macOS : lmms-1.3.0-alpha.1.191%2Bg597f3a4c5-mac10.14.dmg (build link)

Linux

• Linux (AppImage): lmms-1.3.0-alpha.1.191%2Bg597f3a4-linux-x86_64.AppImage (build link)

Windows

• Windows 32-bit: lmms-1.3.0-alpha.1.191%2Bg597f3a4c5-mingw-win32.exe (build link)
• Windows 64-bit: lmms-1.3.0-alpha.1.191%2Bg597f3a4c5-mingw-win64.exe (build link)
• Windows 32-bit: lmms-1.3.0-alpha-msvc2017-win32.exe (build link)
• Windows 64-bit: lmms-1.3.0-alpha-msvc2017-win64.exe (build link)

🤖

{"platform_name_to_artifacts": {"macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://14419-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.191%2Bg597f3a4c5-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14419?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://14418-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.191%2Bg597f3a4-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14418?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://14417-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.191%2Bg597f3a4c5-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14417?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://14421-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.191%2Bg597f3a4c5-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14421?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/rcu0cokhboaj2s5y/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/40116047"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/sr4hl22wgwu7cc3v/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/40116047"}]}, "commit_sha": "f568db7f8090f1d93bc7d5a19c40ab75ab8c149c"}

[irrenhaus3]

Org info: #6076 is a prerequisite for this PR; this PR is a prerequisite for #6086.

[PhysSong]

I think getPluginFactory and getGUI may be defined as inline functions in header files.

[irrenhaus3]

I think getPluginFactory and getGUI may be defined as inline functions in header files.

Yeah, I was kind of uncertain on this one in regards to impact on compile time VS link time. But I couldn't make out any substantial time difference between the two approaches, so I'd go with inlining as well. Makes it easier to use them without accidental linking errors.

[irrenhaus3]

Since there are a lot of commits now and they're becoming hard to read, here's a higher-level changelog of all changes so far:

• Remove redefinition of assert in non-debug builds
• Replace NULL with nullptr in all first-party files (includes a second-hand change in the MidiImport plugin's portsmf/Allegro embed, where an out-of-bounds read now throws std::out_of_range instead of calling assert(false))
• Replace macros gui and pluginFactorywith functionsgetGUI()andgetPluginFactory()`, respectively
• Replace macros MM_ALLOC and MM_FREE with function templates
• Replace automation node access macros INVAL, OUTVAL etc. with equivalent functions
• Remove redefinition of exp10 as a macro, replace occurences with std::pow

[Veratil]

(includes a second-hand change in the MidiImport plugin's portsmf/Allegro embed, where an out-of-bounds read now throws std::out_of_range instead of calling assert(false))

Since this is technically a third party library, let's not change anything in here. I'm working with a new upstream fixing a lot and will eventually be removing this folder and using the upstream.

Replace automation node access macros INVAL, OUTVAL etc. with equivalent functions

Have you tested this change and made sure it works as before?

[irrenhaus3]

Since this is technically a third party library, let's not change anything in here. I'm working with a new upstream fixing a lot and will eventually be removing this folder and using the upstream.

Agreed. Depending on which we want to merge first, I can revert the refactor in portsmf back to the NULL macro, or rebase onto your changes once they've been merged.

Have you tested this change and made sure it works as before?

Not extensively, I only did a GUI session running some automation. I also ran the test suite, but that never calls into any of these functions. It's not a big deal in this particular instance since the changes are very easy to reason about, but I could add a unit test just to be sure. As for the changes:
In the example of INVAL, the current implementation is the single-line function return it->getInValue() where it is a const iterator over AutomationPattern::timeMap. The previous implementation was a macro INVAL(x) expanding to (x).value().getInValue(), called exclusively on const iterators over AutomationPattern::timeMap. Calling value() on that iterator returns a const ref to the visible value, which is also the effect of operator-> on const iterators. So the two implemetations are exactly equivalent here, save for the newly introduced extra function call, which the compiler can inline during optimization.

Hope that eliminates concerns about this change - but of course, a unit test would be better than taking my word for it, so if we can define what the test should look like, I'll add it to the test suite. :)

[Veratil]

I wasn't too worried just wanted to make sure the INVAL and those macros changes still worked since we were trying to figure out a good way to replace all the calls. Better solutions are always accepted! 😁

For portsmf, at this point it shouldn't be too much of a problem since the folder will be removed eventually. Your choice to revert or not.

[JohannesLorenz]

What's the state here?

[irrenhaus3]

Some futher work hadn't been pushed yet, but other than that it was left lying around for a bit because I was busy with other things. Over all, a great deal of macros have been replaced now, though, so it might be ready to merge after a few more commits.

[irrenhaus3]

Requesting sanity test on Windows for commit ae30a57. Verify that multi-byte character strings are widened correctly in F_OPEN_UTF8.

[irrenhaus3]

The following macros have not been replaced in this PR:

ladspa.h
All macros left intact.
Reason:
Part of our tree, but third-party.

lmms_basics.h
STRINGIFY and STR
Reason:
Used in codegen for plugins and other compile-time string constructions; replacing these would imply a bigger refactor than this PR justifies.

AudioPortAudio.h
paContinue, paComplete etc. - everything that becomes active if PORTAUDIO_V19 is undefined.
Reason:
Older versions of Portaudio require these symbols to exist. Removable once we stop supporting these versions.

AutomatableModel.h
mapPropertyFromModel etc., and MODEL_IS_VISITABLE
Reason:
Used for code generation / boilerplate ease in implementing AutomatableModel visitors. Modern IDEs make this boilerplate generation less painful, but for anyone not on such an IDE, these might still be useful.

LmmsPalette.h
ACCESSMET
Reason:
Local codegen macro used to generate getter/setter functions. Doesn't exist anymore beyond the scope of that boilerplate section.

MemoryManager.h
MM_OPERATORS
Reason:
These are overloads for the new and delete operators that call into LMMS' own memory allocator. It would be possible to avoid this macro if we went the stdlib way of making the allocator a template parameter of classes that may implement it, but this would be a bigger refactoring effort than this PR justifies.

[irrenhaus3]

Summary

This is a concise summary of the changes implemented in this PR, listing as many macro replacements as I was able to gather from the commit log and diff. Possible missing items not ruled out, though ;-).

• NULL -> nullptr
• gui -> Function getGUI()
• pluginFactory -> Function getPluginFactory()
• assert (redefinition) -> using NDEBUG instead, which standard assert respects.
• powf (C stdlib symbol clash) -> removed and all expansions replaced with calls to std::pow.
• exp10 (nonstandard function symbol clash) -> removed and all expansions replaced with calls to std::pow.
• PATH_DEV_DSP -> File-scope QString of identical name and value.
• VST_SNC_SHM_KEY_FILE -> constexpr char* with identical name and value.
• MM_ALLOC and MM_FREE -> Functions with identical name and implementation.
• INVAL, OUTVAL, etc. for automation nodes -> Functions with identical names and implementations.
• BandLimitedWave.h: All integer constant macros replaced with constexpr ints of same name and value.
• FAST_RAND_MAX -> constexpr int of same name and value.
• QSTR_TO_STDSTR -> Function with identical name and equivalent implementation.
• CCONST -> constexpr function template with identical name and implementation.
• F_OPEN_UTF8 -> Function with identical name and equivalent implementation.
• LADSPA_PATH_SEPARATOR -> constexpr char with identical name and value.
• UI_CTRL_KEY -> constexpr char* with identical name and value.
• ALIGN_SIZE -> Renamed to LMMS_ALIGN_SIZE and converted from a macro to a constexpr size_t.
• JACK_MIDI_BUFFER_MAX -> constexpr size_t with identical name and value.
• versioninfo.h: PLATFORM, MACHINE and COMPILER_VERSION -> prefixed with LMMS_BUILDCONF_ and converted from macros to constexpr char* literals.
• Header guard _OSCILLOSCOPE -> renamed to OSCILLOSCOPE_H
• Header guard _TIME_DISPLAY_WIDGET -> renamed to TIME_DISPLAY_WIDGET_H
• C-style typecasts in DrumSynth.cpp have been replaced with static_cast.
• constexpr numerical constants are initialized with assignment notation instead of curly brace intializers.
• In portsmf, Alg_seq::operator[] will throw an exception instead of returning null if the operator index is out of range.

Additionally, in many places, global constants that were declared as const T foo = bar; were changed from const to constexpr, leaving them const and making them potentially evaluable at compile time.

Some macros that only appeared in single source files and were unused in those files have been removed entirely.

[irrenhaus3]

Alas, history hath been rewritten.

[Veratil]

This change doesn't match the previous commit format for other constexpr. In the previous name = value is used while this is using name{value}.

[Veratil]

Unless I haven't hit a commit that changes this yet, let's change these (float)... C casts to C++ static_cast<float>(...).

[Veratil]

Another name{value}.

[Veratil]

I just realized this isn't linking the commit I'm commenting on. 😠

[JohannesLorenz]

This PR had 2 reviews. Merging in 2 days if no one else is opposed (or merges it before I do).