Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing FluidSynth checks, consistency tweak #2109

Merged
merged 3 commits into from
Dec 31, 2024

Conversation

MrAlaux
Copy link
Collaborator

@MrAlaux MrAlaux commented Dec 30, 2024

Should fix this Doomworld report.

While we're on this, I noticed that the following check uses #ifdef instead of #if defined:

woof/src/i_oalmusic.c

Lines 612 to 617 in 1640988

#ifdef HAVE_FLUIDSYNTH
if (active_module == &stream_fl_module)
{
return midiplayer_fluidsynth;
}
#endif

Why? Should we change it or all the others for consistency?

@fabiangreffrath
Copy link
Owner

Why? Should we change it or all the others for consistency?

#ifdef and #if defined() are completely synonymous, but I agree it would be nice to use only one way of writing for consistency. I prefer the latter, because it's possible to combine it with other checks, e.g. #if defined(foo) && defined(bar).

@MrAlaux
Copy link
Collaborator Author

MrAlaux commented Dec 30, 2024

#ifdef and #if defined() are completely synonymous, but I agree it would be nice to use only one way of writing for consistency. I prefer the latter, because it's possible to combine it with other checks, e.g. #if defined(foo) && defined(bar).

So, do I make the change in this PR?

@fabiangreffrath
Copy link
Owner

So, do I make the change in this PR?

Yes, please.

@MrAlaux
Copy link
Collaborator Author

MrAlaux commented Dec 30, 2024

So, do I make the change in this PR?

Yes, please.

Done in a720d33.

@fabiangreffrath
Copy link
Owner

Thank you!

I can confirm that the engine now compiles and runs if fluidsynth is not installed at build time. However, could you please address these warnings as well:

[69/188] Building C object src/CMakeFiles/woof.dir/i_oalmusic.c.o
/home/fabian/Source/woof/src/i_oalmusic.c:442:12: warning: ‘fl_gain’ defined but not used [-Wunused-variable]
  442 | static int fl_gain, opl_gain;
      |            ^~~~~~~
[135/188] Building C object src/CMakeFiles/woof.dir/mn_setup.c.o
/home/fabian/Source/woof/src/mn_setup.c:2495:13: warning: ‘SetMidiPlayerFluidSynth’ defined but not used [-Wunused-function]
 2495 | static void SetMidiPlayerFluidSynth(void)
      |             ^~~~~~~~~~~~~~~~~~~~~~~

@MrAlaux
Copy link
Collaborator Author

MrAlaux commented Dec 30, 2024

However, could you please address these warnings as well

Will do tomorrow.

@MrAlaux MrAlaux changed the title Add missing FluidSynth check Add missing FluidSynth checks, consistency tweak Dec 31, 2024
@MrAlaux
Copy link
Collaborator Author

MrAlaux commented Dec 31, 2024

Will do tomorrow.

Done in 6957e20.

Copy link
Owner

@fabiangreffrath fabiangreffrath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you!

@fabiangreffrath fabiangreffrath merged commit 4e37793 into fabiangreffrath:master Dec 31, 2024
8 checks passed
@MrAlaux MrAlaux deleted the woof-fl_gain branch December 31, 2024 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants