Default-initialize `RBJbase`'s `state` member

[kcgen]

The DllExport of RBJbase generates a default constructor, Iir::RBJ::RBJbase::RBJbase(), which leaves RBJbase's state member uninitialized.

Reported by @FeralChild64 on Gentoo Linux, AMD64, GCC 11.3.0:

https://github.com/dosbox-staging/dosbox-staging/pull/1820#issuecomment-1197252870

iir1-1.9.3/iir/RBJ.h: In constructor 'constexpr
Iir::RBJ::RBJbase::RBJbase()':

iir1-1.9.3/iir/RBJ.h:66:26: warning:
   'Iir::RBJ::RBJbase::state' should be initialized in the
   member initialization list [-Weffc++] 66 | struct DllExport
   RBJbase : Biquad

[berndporr]

We have been here before: that -Weffc++ is a dumb 10 year old warning. State is initialised anyway because we fixed that before. So the only thing what it will do is shutting up your compiler warning. But in order to do that you'll instantly demand a new release of IIR1 which then forces me to test it on various architectures, make new packages, etc etc. So, sorry. Rejected.

[berndporr]

For reference: https://stackoverflow.com/questions/11496942/understanding-weffc

[kcgen]

hey @berndporr ,

I hear you; code churn to pacify compiler warnings - especially those like -Weffc++ that are often lambasted, can seem like a fools errand. Some projects don't care about this stuff.

The DOSBox-Staging project almost failed to get Adlib Gold working reliably after a ton of excellent work from @johnnovak while also leveraging Iir1:

We were faced with the YM7128B emulator steadily going off the rails:

-Weffc++ is a dumb 10 year old warning

It might be dumb and old, but -Weffc++ indicated the YM7128B chip emulator had a handful of uninitialized members, which after fixing, allowed Adlib Gold emulation to work reliably.

I would not waste our time if I didn't believe there was value for both of us. I know you care about these things:

• ca60fa9 1 year, 3 months ago Bernd Porr @sven-herrmann pointed out that m_normalW is not initialised.

• 09afb2e 2 years, 6 months ago Bernd Porr Fixed a couple of uninitialised variables

Likewise, Iir1 has consistently applied default initializes for all non-scalar members:

Cascade.h:

• Biquad m_stages[MaxStages] = {};
• StateType m_states[MaxStages] = {};

Layout.h:

• PoleZeroPair m_pairs[(MaxPoles+1)/2] = {};

PoleFilter.h:

• LayoutBase m_digitalProto = {};
• AnalogPrototype m_analogProto = {};
• Layout <MaxAnalogPoles> m_analogStorage = {};
• Layout <MaxDigitalPoles> m_digitalStorage = {};

So I thought one last one would be acceptable and consistent:

RBJ.h:

• DirectFormI state = {};

State is initialised anyway because we fixed that before. So the only thing what it will do is shutting up your compiler warning.

Indeed the Iir::DirectFormI class's own double members have default initializers, so yes, leaving DirectFormI state; uninitialized will still fall back to its default constructor.

However, the value with DirectFormI state = {} is that we guarantee its initialization regardless if DirectFormI is a class, struct, or even a typedef to a scalar value. This "don't trust the type" approach is robust in large code-bases when types are being refactored.

The compiler is allowed to collapse all intermediate temporaries, so these operations aren't wasted - it just becomes a single assignment.

you'll instantly demand a new release of IIR1

Probably true: :laughing:

But me asking for a release is a separate matter from the code-base - and you can promptly tell me to a take a hike because it's all too much work. I can easily disable the warning prior to including the Iir1 header.

And to be extra clear: no, I won't ask for release, and no, I won't passively expect it. You've already done a lot in the recent months to get Iir1 happily working w/ Meson's wraps - which we've finally put to use. And I appreciate all of that! I am content to give it /x-more years/ until the next major release.

[berndporr]

If it makes you happy. As I said I won't create a release.

[kcgen]

Much appreciated, @berndporr.

[berndporr]

You are right that there are times when I'm bored I do a bit of cosmetics but then the filter code wise has been rock solid for years. In the old days class variables couldn't even be initialised and the code is from these old days so the original author has already done a stellar job setting the right values. For example that the gain is not initialised actually won't matter because it's always set when then coefficients are calculated. The filter has no biqads when init is not called so there won't be any gain etc. Personally I'd love actually rather somebody who knows signal processing and not just looks at code and cares about compiler warnings. Or suggests performance improvements.

[berndporr]

Just looking at all pull request they are either about compiler warnings or cmake features. I've just writen python code for A/B/C weighting filters. Now anybody up for converting them to C++? Or perhaps adding the machted z-transform or adding pure integer arithmetic? I have a lot of ideas but little time...

[kcgen]

DOSBox-Staging now has a significant amount of audio processing (resampling, upsampling, band-filtering, and soon a re-implementation of soft-liming/compression) at the tail end of our single-threaded emulation loop.

And it goes beyond just a stereo signal: DOSBox emulates many audio devices that games often used simultaneously: PC Speaker + OPL, or Sound Blaster + MIDI, and so on. So we often are running back-to-back processing loops for multiple devices before sending the audio out to SDL.

It's very likely that all of this is going to impact performance on lower end hardware (like the Raspberry Pi), and we'll need to look into speeding it up and/or spawning them into threaded workers. My hunch is this is all just the beginning of more attention (from DOSBox-Staging) coming Iir1's way :partying_face: , and I also hope it's regarding things far more interesting than compiler warnings!

Thanks for expressing your longer term vision for the project; specifically that you're open to more signal processing improvements, refactoring, and other non-trivial changes and enhancements.