On 23.09.24 10:46, Will Godfrey wrote:
We forgot to test headless build,
...and what is most annoying, I had that on my list, but somehow
in the fury of the last parts of integration I overlooked that. Most
of the other things on this list were ideas and possible improvements
or concerns that had been sorted out meanwhile.
and that's pretty important to one of our users who reported this:
yoshimi/src/Interface/InterChange.cpp:107:56: error: ‘toGUI’ was not
declared in this scope
107 | guiDataExchange{[this](CommandBlock const& block){
toGUI.write(block.bytes); }},
So, drawing from the way matters are assumed to work, then a headless build
should actually never use the guiDataExchange. And while it would probably
by quite difficult to remove (hide by compile exclusion) the guiDataExchange
member, since we would have to hide/exclude then a lot of further code in
the core, what we can do is define another Lamda function in this case,
which just does nothing. I.e.
#ifdef GUI_FLTK
///.................original code here
#else
guiDataExchange{[this](CommandBlock const& block){ /* do nothing */ }},
#endif
Another issue that was pointed out on the Github pull request
https://github.com/Yoshimi/yoshimi/pull/207#pullrequestreview-2322393639
As part of my rework, I used some generic helpers and created a new header
Util.hpp. There is a function using std::set in this header, and basically
I wanted to avoid including <set> directly, since then this include would
be pulled into a large part of the whole code base.
The problems seem to arise only with some newer versions of the standard
library, where the forward declaration I provided seemingly generates an
ambiguity. Now I am unsure how to proceed best.
The reporter of the problem suggests a cheap solution, just bite the
bullet and include the damn header. They even propose additionally to include
<algorithm>, and I do absolutely not understand why this would be necessary.
Basically I see the following avenues to resolve the problem
(1) the careless solution, just #include <set>, maybe even <algorithm>
and don't give a shit about compilation times. Personally I would
totally reject that idea, because for me this is the typical mindset
of a juvenile programmer, who pulls in half the internet just to
avoid using one's own brain even for the most simple stuff.
But it is very much possible that I am overreacting here and acting
as a typical "old timer".
(2) attempting to fix the ambiguity, possibly by also adding a suitable
forward declaration for polymorphic_allocator. This would require
to find out what version of the library actually causes the problem
and then working from there (using a docker container to test the build)
(3) reconsidering the usage of the "contains()" function. Yes, personally
I like to use such a shorthand, because IMHO it makes the code clearer.
But that's me. Right now we have only one single usage, which is in
InstanceManager
uint InstanceManager::SynthGroom::allocateID(uint desiredID)
{
if (desiredID >= 32 or (desiredID > 0 and contains(registry, desiredID)))
desiredID = 0; // use the next free ID instead
if (not desiredID)
{
....
It would easy to resolve that by just defining "contains()" in an anonymous
namespace in InstanceManager.cpp. However, that would somehow defeat the purpose
of using such short-hand utilities, because then, over time, several definitions
of such tiny short-hand functions would spread out over the code base.
So this is IMHO to quite some degree a matter of coding style and taste, and we
should discuss our attitude towards using such short-hand functions. As said,
it is always simple to code the functionality up explicitly; the short-hand
function just makes the intention more clear at usage site (it tells the
reader *what* you want, not *how* it's done)
-- Hermann
_______________________________________________
Yoshimi-devel mailing list
Yoshimi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/yoshimi-devel
