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

Eliminated build warnings #60

Closed
wants to merge 1 commit into from
Closed

Conversation

logiclrd
Copy link

@logiclrd logiclrd commented Sep 6, 2024

This PR eliminates build warnings on a Linux build using GCC version 13.2.0-23ubuntu4, as of September 2024.

  • In the code that uses FluidSynth, there are calls to functions for setting parameters that have been deprecated. This PR splits these into calls to the recommended replacements.
  • There are several calls to fread that ignore the return value. This PR changes the code so that it still ignores the return value, but it does it explicitly with a if statement with an empty body.
  • Marked a couple of switch case blocks as being explicit fallthrough to the next case.

One of the warnings was indicative of an actual bug, I believe. In Ym2612_Nuked.cpp, the definition of the Ym2612_Nuked_Emu::reset function calls OPN2_Reset on chip_r only if chip_r is NULL. I'm pretty sure this should be the other way around, and making it so that it only calls it when chip_r is not NULL eliminates a warning.

With these changes, which I believe do not alter functionality in any way, the ZMusic build runs through with no warnings at all. Could possibly add -Werror to the build.

…parate per-parameter calls in music_fluidsynth_mididevice.cpp. Changed calls to fluid_synth_set_reverb_on and fluid_synth_set_chorus_on to use fluid_synth_reverb_on and fluid_synth_chorus_on.

Suppressed warnings about unused fread return values in music_timidity_mididevice.cpp, music_timiditypp_mididevice.cpp and instrum_dls.cpp.
Corrected indentation in itread.c, readmod.c and readxm.c.
Inlined the functionality of fluid_player_update_tempo inside fluid_player_set_bpm, as the former is deprecated and could be removed in a future version.
Corrected Ym2612_Nuked_Emu::reset in Ym2612_Nuked.cpp to only call OPN2_Reset on chip_r when chip_r is _not_ NULL, instead of only when it is.
Marked case ATTACK and case DECAY in envelope generation in OPL3.cpp as fallthrough to case DECAY and case SUSTAIN respectively.
@logiclrd
Copy link
Author

logiclrd commented Sep 6, 2024

Looks like on the Windows build, the use of [[fallthrough]] requires explicit opt-in to C++17. It has introduced a couple of new warnings into the Windows build. Non-breaking, though.

@logiclrd
Copy link
Author

logiclrd commented Sep 6, 2024

A bunch of warnings still present in the MacOS build. I'm not set up to sort those out. :-P

@coelckers
Copy link
Member

I do not like where this is going, especially the if (fread...) constructs.

@coelckers coelckers closed this Sep 6, 2024
@logiclrd
Copy link
Author

logiclrd commented Sep 8, 2024

I do not like where this is going, especially the if (fread...) constructs.

That is literally the recommended way to indicate that you are explicitly not using the return value, from what I have read.

It used to be to cast the return value to void, but that apparently stopped working (in GCC) a while back. So, some people then started doing (void)(ignored_return_value() + 1). This is obviously ridiculous.

Can you explain what you mean more generally by not liking where things are going? I mean, "where it's going" is being able to treat warnings as errors. Isn't that generally a good thing?

@logiclrd
Copy link
Author

logiclrd commented Sep 8, 2024

Also, the code is presently calling a bunch of deprecated Fluid Synth methods. The message attached to the deprecation explicitly says to call the per-property methods instead

 * @return #FLUID_OK on success, #FLUID_FAILED otherwise
 * @deprecated Use the individual reverb setter functions in new code instead.
 */
int
fluid_synth_set_reverb(fluid_synth_t *synth, double roomsize, double damping,
                       double width, double level)
{

@logiclrd
Copy link
Author

logiclrd commented Sep 8, 2024

Most of the other changes are literally just indenting fixes, but this one is an actual bug fix:

 void Ym2612_Nuked_Emu::reset()
 {
	Ym2612_NukedImpl::ym3438_t *chip_r = reinterpret_cast<Ym2612_NukedImpl::ym3438_t*>(impl);
-	if ( !chip_r ) Ym2612_NukedImpl::OPN2_Reset( chip_r, static_cast<Bit32u>(prev_sample_rate), static_cast<Bit32u>(prev_clock_rate) );
+	if ( chip_r ) Ym2612_NukedImpl::OPN2_Reset( chip_r, static_cast<Bit32u>(prev_sample_rate), static_cast<Bit32u>(prev_clock_rate) );
 }

This code is currently literally saying, "Only call OPN2_Reset on the chip data structure if it's a NULL reference."

A static code analysis warning results, because the reset process involves dereferencing the pointer that's passed in. A warning found this actual bug.

@logiclrd
Copy link
Author

logiclrd commented Sep 8, 2024

I have pushed a commit that handles the ignored fread return values in a different way. I believe you will see it if you reopen the PR.

jerome-trc added a commit to jerome-trc/zmsx that referenced this pull request Sep 8, 2024
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