Am 26.08.21 um 21:09 schrieb Will Godfrey:
Please take a look at the branch 'newargs' As far as I can tell this is
correct for both standalone and LV2. I tested on Ardour, Muse and Qtractor.
Can someone look through this and see if it can be streamlined?
Hi Will,
last week I was away for some days to visit a friend....
Just did a review of your changes since last release up to your branch 'newargs'
Basically matters look correct for me, except for some minor issues.
(1) Starting with the big picture.
As we all know, there is a lot rather non-orthogonal and tangled in the way
Yoshimi treats its configuration, instance management and startup. Most notably
there is no clear dependency or responsibility chain. Rather, objects are
mutually interdependent (Synth, MusicClient, Config), which makes it rather
hard to understand what's going on. Changing that would be possible, but
certainly a larger undertaking. So for now we probably all agree better
to leave that topic aside.
However, with your refactoring you kind-of touched those issues, the symptom
being the list you have to push around, and the fact that the LV2 plugin
needs to be fed with an empty list. If you look at the call graph, the
reason is that this information is handed down through several layers,
just to initialise a Config instance. Funny enough, each SynthInstance
has its own Config instance, while in many cases just use "the" synth
through some global var and reach through to synth->getRuntime().
To make a long story short: to my understanding, unless we refactor
the overall dependencies and architecture here, there is no good way
to avoid that empty list; so let's just live with it (it doesn't hurt).
I haven't been able to work out how to send a pointer of the list right
through from main, via SynthEngine and Config to setup so am actually
*copying* the list via the parameters. I've also had to use a dummy list
for LV2.
globalAllArgs is a static global variable (i.e. only visible within main.cpp).
Thus we can be sure it lives until Yoshimi terminates.
Is there any reason why we not just let SynthEngine and Config pass a reference
argument to this global variable through down into Config::applyOptions ?
This is wasteful, but at the same time shouldn't present any problems as the
list is very short - from zero up the the length of a filename or two.
Indeed. All of this Config stuff is in-memory processing and likely to
be dwarfed by the effort for a single sound buffer allocation, not to
mention the time of generating a single PADSynth wavetable.
(2) So basically what your refactoring does is to segregate the commandline
evaluation into two stages. The first stage is the actual parsing, which
is done by the ARGP library. The result is a set of instructions, which
we pass through to the point where the actual instance configuration
happens. I've checked your refactorings and they look correct, with
some minor exceptions, which I'll detail later.
Talking about data structures.
In this case, a std::map<char,string> would be a way better fit than a list.
Instead of building a command string, and then dissecting it down in
Config::applyOptions, we could just direclty pass the key-value pair.
It is possible to iterate over all settings in a map; in this case,
what you get as an "entry" is always a std::pair<char,string>.
This would probably make the code a bit more elegant, without
changing anything fundamental (the code you wrote is correct!)
Incidentally, since C++11 we can simplify...
for (std::list<string>::iterator it = allArgs.begin();
it != allArgs.end();
++it)
{
string line = *it;
...
by using the more compact "for-each-loop"
for (string line : allArgs)
{
...
This refactoring works for every object, which has a begin() and end() method.
(3) Now about some minor defects and problems...
- the refactored code has broken the new '--null' option, which I introduced
recently for the testing effort. Since this is rather "technical" stuff,
I didn't want to "burn" one of the precious short options. Thus I just
picked some arbitrary non-letter code (in that case 13) to act as a key.
There is no deeper reason behind that number, we can change that as we like.
I see that you translated that number 13 into the string "@:", but down
in Config::applyOptions, the corresponding Case is missing.
Incidentally, this --null backend option saves a lot of startup time, since
Yoshimi doesn't have to probe and connect to any backend. Moreover, when
running under Docker, I saw crashes when Yoshimi attempts to probe Alsa.
Haven't investigated that in more detail yet...
- the flag "bool ret" in main.cpp and in CmdOptions.cpp does not make much
sense. It is set to false in each invocation of the ARGP callback.
And it is set to true, when ARGP detects a positional argument (ARGP_KEY_ARG)
and when ARGP invokes the Callback for final validations (ARGP_KEY_END).
However, since there are always further invocations following those cases,
the changed value doesn't make it to the end, and thus this flag is always
false. Would it be true, then Yoshimi would exit(0) (which would be wrong).
- also the other two flags, cmd and gui are not further used in main.cpp.
The actual handling of showGui and showCli happens still in
Config::applyOptions (as it was before, this is not changed and is OK).
Moreover, the whole ComandOptions is used like a function; this does not need
to be an object at all! We could just change the CommandOptions.h to expose
a single public function
std::list<string> parseCmdline(argc, argv);
and assign the resulting list directly to the global var "globalAllArgs".
As said, the flags cmd, and gui are without effect, and the flag ret
is unnecessary, since ARGP by itself terminates the process in case
of a command-line error.
-- Hermann
_______________________________________________
Yoshimi-devel mailing list
Yoshimi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/yoshimi-devel
