lmms
lmms copied to clipboard
LMMS
C++ Namespace layout
Part of the big LMMS refactor is to set up C++ namespaces for LMMS' symbols, to avoid potential name clashes with its dependencies (which has already happened with ZynAddSubFX). My first quick proposal for a namespace layout guided by LMMS' current directory structure went as follows:
Lmms/
Core/
Audio/ -> namespace lmms::audio
Mixer/ -> namespace lmms::mixer
Midi/ -> namespace lmms::midi
Lv2/ -> namespace lmms::lv2
[anything else] -> namespace lmms::core
Gui/ -> namespace lmms::gui
with some symbol name changes such as MixerWorkerThread -> lmms::mixer::WorkerThread.
This is only a draft and open for discussion; the main points to consider here are:
- Namespaces should not be too fine-grained, nor too coarse. They should group together what belongs together and surprises of which symbol lives in which namespace should be minimized.
- They should not be nested too deeply; anything deeper than one nesting level is a chore to work with (think
std::vectorvs.std::chrono::steady_clock) and leads to an increase inusing-declarations (orusing namespace, which can be easily mis-used).
Also, this is not a very important remark right now, but at some point in the future, C++20's Modules feature might be desirable; and since namespaces and Modules are orthogonal to each other, mixing the two can complicate the codebase. Kind of a reiteration of the first point here: If namespaces are set up to effectively group logical modules together, a potential future refactor into C++20 Modules becomes a great deal easier.
Discussion, GO!
Each time this was proposed in the past, I've been an opponent. I've always felt that it proposes inflation to the code for what I would consider a small chance of necessity and a 0% chance of re-usability. I believed it should be on the 3rd-party libraries to namespace, since LMMS isn't reused in other projects or used as a library elsewhere, I'd felt (historically) it was unwarranted and should become an upstream bug when this occurs, not ours.
MixerWorkerThread->lmms::mixer::WorkerThread
This above, all over the codebase (or even worse, the using keyword) really summarizes my sentiments in a one-liner.
That said, its our developer time that suffers when these crashes occur and I don't actively maintain the C++ portions of the code, nor do I actively maintain any medium or large C++ projects in general, so my opinion is simply that and the pros and cons explained by the OP should we weighed greater by those writing the code. 🍻
That said, this has been requested a lot. Related: https://github.com/LMMS/zynaddsubfx/pull/16, quoting Dom:
I agree with using namespaces to address this - this sort of situation is the entire point of having them in the language.
I believed it should be on the 3rd-party libraries to namespace, since LMMS isn't reused in other projects or used as a library elsewhere, I'd felt (historically) it was unwarranted and should become an upstream bug when this occurs, not ours.
Agreed. namespacing is the task of library vendors, but unfortunately not all libraries do it, so implementing it clientside can act as a mitigation for current name clashes and as a precaution for dependencies we might add in the future. namespacing LMMS would also yield the future possibility of splitting up into a frontend/GUI application and a business-logic library that could be used in other projects without any danger of name clashes.
Regarding granularity as in the lmms::mixer::WorkerThread example, in the draft layout all LMMS symbols would live in the lmms namespace, so for referring to symbols in nested namespaces, the lmms:: prefix could be omitted. So in that context the change would be MixerWorkerThread -> mixer::WorkerThread, which I think is tolerable. But maybe all we need is a single lmms namespace that everything lives in, without further nesting; logical grouping into packages could be done later with C++20 Modules, which are designed for that specific use case. But this decision is probably best left to the active C++ maintainers who have the clearest "big picture" view of the codebase.
Adding namespaces to avoid collisions seems like a good idea to me, but the suggested structure feels a bit too complex. Do we really need separate namespaces for audio, midi and lv2? And is there any benefit to nesting it like lmms::core or lmms::gui? It implicitly sort of creates a 3rd, empty lmms namespace that won't be used for anything (apart from other namespaces)?
I would personally go just with LmmsCore and LmmsGui (or whichever upper/lower case variant is preferred). That way it is relatively short and easy to type and understand, and the distinction between code that needs extra attention (realtime sensitive core) and the rest (gui) would more obvious at all times and harder to miss.
And is there any benefit to nesting it like
lmms::coreorlmms::gui?
Yes. If you want to use the Mixer in GUI code, in one case, you use LmmsCore::Mixer, and in the other, you use core::Mixer (shorter).
This also means that e.g. Lv2 types can reference other Lv2 types without naming their namespace, which means the code itself will get shorter (all "Lv2" prefixes will wanish). Though... if you read Lv2 code referencing Manager, it may be hard to differ for a human that this is the "Lv2-Manager" (which manages Lv2 plugins) or just any general "Manager". Not sure about that point.
namespacing LMMS would also yield the future possibility of splitting up into a frontend/GUI application and a business-logic library that could be used in other projects without any danger of name clashes.
This is something I believe LMMS could benefit from working towards.
Nested namespaces aren't really efficient in C++ from my experience. It's common in C# but in C++ it's usually a one level namespace
Nested namespaces aren't really efficient in C++ from my experience. It's common in C# but in C++ it's usually a one level namespace
I think the main thing to keep in mind there is that C++ namespaces are not packages like in Java or C#; there is no such thing as "namespace visibility" and you can't export or import namespaces. They only exist to avoid name collisions, and thus should be used to achieve this goal without burdening client code too much.
I like he29s suggestion of only splitting into core and gui and I'd like to suggest the following:
namespace lmms{
// Everything lives here
namespace gui{
// The only nested namespace. GUI elements live here.
}
}
That way, all LMMS symbols can just refer to each other without the lmms:: prefix, and GUI symbols can be reached with the three-letter prefix gui::. I think that's pretty painless and achieves what we want from introducing namespaces. Thoughts?
That way, all LMMS symbols can just refer to each other without the
lmms::prefix, and GUI symbols can be reached with the three-letter prefixgui::. I think that's pretty painless and achieves what we want from introducing namespaces. Thoughts?
I like this variant better than my original suggestion; it gets the job done, it's shorter, gets out of the way most of the time, and still achieves the core / GUI separation. Thumbs up.
At least, it makes it harder to use gui elements in the core now (and easier to find those later). If we really think it is necessary to have a nested core later, it can still be added.
What should we do for plugins? Some options:
- Root LMMS namespace
lmms:: - Shared plugin namespace
lmms::plugin:: - Individual namespaces
lmms::tripleoscillator:: - Individual namespaces in plugin namespace
lmms::plugin::tripleoscillator::
Long namespace names aren't as much of a concern here since most plugins aren't referenced elsewhere in the code.
Also, plugins have core and GUI components too. Should these go in separate namespaces?
Another question that needs to be cleared up: For UI widgets following MVC patterns, there's multiple classes involved; Widget and WidgetModel (good example: ComboBox). Should the model classes live in the namespace lmms::gui too? I would argue yes, since they're paired with a specific widget, but there's an argument for no as well, since they don't actually need to be paired with that particular widget; a different widget could display the same data a different way.
One way or the other, we'll need to add gui:: namespace prefixes to some member variable declarations, so I think it boils down to which of the two options complicates this matter the least.
What should we do for plugins? Some options:
* Root LMMS namespace `lmms::` * Shared plugin namespace `lmms::plugin::` * Individual namespaces `lmms::tripleoscillator::` * Individual namespaces in plugin namespace `lmms::plugin::tripleoscillator::`Long namespace names aren't as much of a concern here since most plugins aren't referenced elsewhere in the code.
Also, plugins have core and GUI components too. Should these go in separate namespaces?
Regarding this question, I would generally advise against nesting namespaces too much, so I'd argue for the lmms::3osc schema here, with either lmms::3osc::gui or lmms::gui::3osc to pair.
Regarding this question, I would generally advise against nesting namespaces too much, so I'd argue for the
lmms::3oscschema here, with eitherlmms::3osc::guiorlmms::gui::3oscto pair.
I thought we agreed on using only a minimal amount of namespaces, lmms and lmms::gui?
Should the model classes live in the namespace
lmms::guitoo?
In theory, there could be a different GUI accessing the same models (in practice, very unlikely?). If we introduce lmms::models, this might require adding different namespaces to headers containing both gui and model.
Minimizing complication sounds best to me, too, whatever the solution is.
@irrenhaus3 Is anything blocking you with this? It's a useful PR and is theoretically on the re-org list (though we agreed we can skip it).
I'm available to solve the merge conflicts if that's the only thing needed to get this PR through...
About the whole discussion of MixerWorkerThread -> lmms::mixer::WorkerThread:
I think this is really easily done with a simple shell/python script. Pseudo code:
For all classes c in LMMS:
if c == "Mixer...", and "..." is not a class in LMMS
Replace c to "mixer::..." in all files
Also, I don't see issues like with "using" here, if you do it like above. Let's say you have Lv2Plugin and Plugin. The above code will not turn Lv2Plugin into lv2::Plugin, since Plugin already exists.
What do others think? Do you see any risks in this approach?
Just a reminder: Mixer at the point of issue creation is now renamed to AudioEngine.
Namespaces only exist to avoid name collisions
What's the deal with namespacing Mixer? Threads?
What should we do for plugins?
The only reason I see for a plugin namespace is to protect the plugin itself from breaking if the core introduces a conflicting name:
// core.h
class Model {}; // This class got introduced in core some time after the plugin was written
// old_plugin.h
#import <core>
class Model {}; // Error: redefinition of Model
Plugins shouldn't include symbols from other plugins, so there should be no need for things like lmms::3osc, right?
the distinction between code that needs extra attention (realtime sensitive core) and the rest (gui) would more obvious at all times and harder to miss
namespacing LMMS would also yield the future possibility of splitting up into a frontend/GUI application and a business-logic library that could be used in other projects without any danger of name clashes.
This could be a good motivation for lmms::plugins::gui. Inside the plugin core you'd be forced to write gui::FrontendThing which tells you this is probably a bad idea. And inside the plugin GUI you can just use BackendThing without thinking about it.
Although I'm not sure how well all plugins are separated into core/gui right now. It could be quite tedious to do this separation.
After thinking about it I don't see a point for lmms::plugins
The purpose of the lmms namespace was to separate our code from conflicts in the Zyn code which we had no control over. Now if a name in a plugin creates a conflict, we can simply change it. And if someone writes a third-party plugin there's no guarantee they'll even create a namespace. Anyhow they shouldn't be adding stuff in the lmms namespace.
