Outsource a few GLIB related functions to fluid_sys

[MrSapps]

No description provided.

[mawe42]

Thanks for your contribution! Before we dive in to discuss the actual changes, please make sure that your commits are nicely separated and on topic. There are numerous changes that fix typos, change example code, formatting changes etc. While those smaller changes are probably very welcome, please provide them in a separate PR and keep this one focussed only on the glib stuff. Thanks!

[MrSapps]

@mawe42 thanks - this is now only the GLIB changes, opened 2 more PRs for warnings + typo fix

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

38.5% Coverage
0.0% Duplication

[mawe42]

Regarding the actual changes to make glib replaceable... I think it's a start, but I have to admit I'm not really fond of the many #ifdefs for GLIB_SUPPORT in the code. I would rather see a new include that bundles the whole glib stuff into a single file. I also don't like the hardcoded "glib_replacement.h" in fluidsynth_priv.h.

If we were to officially include such an option to build FluidSynth without glib, then users would expect us to officially support, maintain and document this use-case. Which means we need documentation, a clear example of what the "glib-replacement" needs to provide. Expecting people to hunt around in the code and glib docs to find out which functions and interfaces they need to provide is probably going to backfire on us. Don't get me wrong... it's great that you can remove glib and provide your own replacement with such a small changeset. But that's a completely different thing compared to us officially including it.

So I still think a more comprehensive approach is needed here. Refactor all platform related / glib code into an abstraction layer. Write documentation for the interface of that layer, functions, calling conventions. Then write an example replacement platform layer that people can modify to their needs. And maybe even provide a cmake option to allow users to specify their own source for that layer, ideally even out-of-tree to keep their fork nice and clean.

TLDR: in my opinion, this is not ready for merge and more discussion on the actual approach would be necessary.

@derselbst What's your take on this?

[MrSapps]

Is it possible to have wrapper functions for the atomic stuff? I figured that would make it not atomic anymore if there is a function call between them?

If there has to be an example implementation what should it use? SDL2? A Win32 impl?

[mawe42]

Good questions, which would all need to be discussed. Maybe we should continue throwing ideas around in #847.

And please don't get me wrong: I'm personally not opposed to making changes to the FS code to make it easier for people to "hack away" glib. But introducing a cmake option to disable glib and having a placeholder header file hardcoded somewhere is crossing the border towards "new feature". A feature that directly touches on the matters we are currently discussing in #847 and where we have not reached a good solution yet.

[derselbst]

I must admit that I don't like this change too much either for the reasons you've just mentioned. However, I could befriend with this PR if we agree to move on to C++11 anyway. Because then I would see this only as temporary change, which is not worth to engineer any further.

I will make a new comment in #847 in a few seconds. And then give it some time so we see how the feedback is and we better know in which direction we should go.

[MrSapps]

Btw @derselbst / @mawe42 what do you think of this PR without the cmake change/ifdefs? (i.e just pushing all of the glib stuff into the priv/sys header?).

[mawe42]

just pushing all of the glib stuff into the priv/sys header?

Sounds good to me. That is actually what I expected you would propose in this PR after reading the discussion in #899 :-)

[derselbst]

stuff into the priv/sys header?

Sounds good to me. That is actually what I expected you would propose in this PR after reading the discussion in #899 :-)

I second that.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

38.5% Coverage
0.0% Duplication

I fixed it

[derselbst]

I implemented what Marcus and I had in mind. Should be good now. Thanks.