Reduce build warnings ( [-Wmissing-field-initializers] )

[henrygab]

Version/revision of the library used

v2.8.0

Describe the bug

Build warnings caused by initialization of a structure without the requisite fields in ir_Kelon.cpp line 443.

To Reproduce

Use the library. Enable all warnings in GCC via -Wall -Wextra.

Actual results

Collapsed simplified build warnings

...\IRremoteESP8266\src\ir_Kelon.cpp: In member function
'stdAc::state_t IRKelonAc::toCommon(const stdAc::state_t*) const':

...\IRremoteESP8266\src\ir_Kelon.cpp:443:25: 
stdAc::state_t result{};
                      ^

warning: missing initializer for member 'stdAc::state_t::protocol'
warning: missing initializer for member 'stdAc::state_t::model'
warning: missing initializer for member 'stdAc::state_t::power'
warning: missing initializer for member 'stdAc::state_t::mode'
warning: missing initializer for member 'stdAc::state_t::degrees'
warning: missing initializer for member 'stdAc::state_t::celsius'
warning: missing initializer for member 'stdAc::state_t::fanspeed'
warning: missing initializer for member 'stdAc::state_t::swingv'
warning: missing initializer for member 'stdAc::state_t::swingh'
warning: missing initializer for member 'stdAc::state_t::quiet'
warning: missing initializer for member 'stdAc::state_t::turbo'
warning: missing initializer for member 'stdAc::state_t::econo'
warning: missing initializer for member 'stdAc::state_t::light'
warning: missing initializer for member 'stdAc::state_t::filter'
warning: missing initializer for member 'stdAc::state_t::clean'
warning: missing initializer for member 'stdAc::state_t::beep'
warning: missing initializer for member 'stdAc::state_t::sleep'
warning: missing initializer for member 'stdAc::state_t::clock'

Example code used

Any code using v2.8.0 of this library, when set to compile with all warnings, should expose this bug.

Expected behaviour

Libraries should build cleanly at all warning levels.

Output of raw data from [IRrecvDumpV2.ino]

N/A

What brand/model IR demodulator are you using?

N/A

Circuit diagram and hardware used (if applicable)

N/A

I have followed the steps in the Troubleshooting Guide & read the FAQ

Yes ... not applicable to this type of bug

Has this library/code previously worked as expected for you?

N/A ... first work to integrate this.

Other useful information

Proposed fix:
Just use the default constructor for stdAc::state_t:

stdAc::state_t result{};

Becomes:

stdAc::state_t result;

This appears to have the correct results. The default constructor appears to zero-initialize all fields in the structure. Each of the embedded enum types, when set to zero, appears to have a reasonable value. And, the result is substantially identical to the expressed intent of the writer of this code.

I'll prep a PR for this....

[mcspr]

@henrygab what build environment are you running, and why there were no warnings in CI?
Can't find any mentions of state_t for the commit just before yours... unless I am missing something in the search.
https://github.com/crankyoldgit/IRremoteESP8266/runs/4442634611 (test suite)
https://github.com/crankyoldgit/IRremoteESP8266/runs/4442634605 (pio-run for examples)

Code initializes the struct regardless, so it is sort-of a non issue, I am just a bit confused by the conclusions here :)

c/p into godbolt does not produce anything of sorts (also notice memset at the start that disappears without the braces)
https://godbolt.org/z/73r6v11Yh
https://godbolt.org/z/d1KsdKe8q

afaik POD structs are slightly different from normal classes in cases like this and nothing is done to the members without default value that we provided ourselves (i.e. int member { 12345 };)
https://en.cppreference.com/w/cpp/language/default_initialization
https://en.cppreference.com/w/cpp/language/zero_initialization

[henrygab]

Hi @mcspr,

First, yes you are correct ... this is sort-of a non-issue. The old code should have had correct behavior, because it was aggregate initialization falling through to value-initialization, which (as you note) for this struct is zero-initialization. The new code is default initialization, which for this struct is (as you note) exactly the same result.

The build environment that was used:

PlatformIO, with a project that used arduino_core_2_7_4 / [email protected] to compile for ESP8266 boards.
The main project depot is at jasoncoon/esp8266-fastled-webserver. The specific branch (PR is pending) where IR can be enabled by adding -D ENABLE_IR=1 to a specified build environment (in platformio.ini or platformio_override.ini) is HACKER-3000/esp8266-fastled-webserver.

Motivations

I reported this because it produced warnings during build. I am not defending the decision to report this as a warning, since I cannot fathom the reasoning behind it. I've seen GCC add warnings for things that are bad habits or risky, even if not technically wrong. I didn't look into whether there is such a justification here. When two equally-correct alternatives exist, and they result in identical code, but one causes build warnings ... I consider that worthy of a filed issue.

I filed this issue to raise awareness, and provide the opportunity to review. Maybe the fix won't happen ... in the end, that's not my decision. At the same time, I wry to provide information to help make an informed decision. I'm comfortable with the likelihood that I won't always agree with those decisions. 🙂

Let me know if there's more information I can provide....

[henrygab]

@mcspr -- Max,

After digging deeper, I think GCC got it wrong. Brace-initialization should not have caused an error, even with pedantic warnings enabled. Worse, I think you were right ... this doesn't zero-initialize the fields.

I was overly trusting in GCC, and didn't dig deeply enough. The PR I proposed (and that was merged) may cause some uninitialized memory to be returned. This is "not good".

I will immediately create a PR to revert this change. I will line to this issue (and the original PR).

THANK YOU for asking the questions when something didn't seem right. My brain should also have flagged this.

Mea culpa!

P.S. - thanks for helping me notice that godbolt added the ESP32 compilers. That's a great addition to what was already an invaluable tool!

[crankyoldgit]

FWIW, a "better" (IMHO) solution would be use this instead:

stdAc::state_t result;

or we create a function that returns a safely (default) initialised state_t, such that we could say stdAc::state_t result = stdAc::initState();

either one of those and then remove the redundant/duplicate setting of default values later in the method.

Or ... use:

IRremoteESP8266/src/IRac.cpp

Lines 78 to 110 in 31432b4

	/// Initialise the given state with the supplied settings.
	/// @param[out] state A Ptr to where the settings will be stored.
	/// @param[in] vendor The vendor/protocol type.
	/// @param[in] model The A/C model if applicable.
	/// @param[in] power The power setting.
	/// @param[in] mode The operation mode setting.
	/// @param[in] degrees The temperature setting in degrees.
	/// @param[in] celsius Temperature units. True is Celsius, False is Fahrenheit.
	/// @param[in] fan The speed setting for the fan.
	/// @param[in] swingv The vertical swing setting.
	/// @param[in] swingh The horizontal swing setting.
	/// @param[in] quiet Run the device in quiet/silent mode.
	/// @param[in] turbo Run the device in turbo/powerful mode.
	/// @param[in] econo Run the device in economical mode.
	/// @param[in] light Turn on the LED/Display mode.
	/// @param[in] filter Turn on the (ion/pollen/etc) filter mode.
	/// @param[in] clean Turn on the self-cleaning mode. e.g. Mould, dry filters etc
	/// @param[in] beep Enable/Disable beeps when receiving IR messages.
	/// @param[in] sleep Nr. of minutes for sleep mode.
	/// -1 is Off, >= 0 is on. Some devices it is the nr. of mins to run for.
	/// Others it may be the time to enter/exit sleep mode.
	/// i.e. Time in Nr. of mins since midnight.
	/// @param[in] clock The time in Nr. of mins since midnight. < 0 is ignore.
	void IRac::initState(stdAc::state_t *state,
	const decode_type_t vendor, const int16_t model,
	const bool power, const stdAc::opmode_t mode,
	const float degrees, const bool celsius,
	const stdAc::fanspeed_t fan,
	const stdAc::swingv_t swingv, const stdAc::swingh_t swingh,
	const bool quiet, const bool turbo, const bool econo,
	const bool light, const bool filter, const bool clean,
	const bool beep, const int16_t sleep,
	const int16_t clock) {

e.g.

stdAc::state_t result = stdAc::initState(&result, decode_type_t::KELON, -1, (prev == nullptr || prev->power) ^ _.PowerToggle, toCommonMode(getMode()), getTemp(), true, ... etc etc);

[mcspr]

At this point, why not implement state_t::state_t() with initializer list?

// header
struct state_t {
  ...
  state_t();
};

// cpp
state_t::state_t() :
  vendor(decode_type_t::UNKNOWN),
  model(-1),
  power(false),
  ...
{}

Or just resort to the header, but idk how would you would feel about keeping defaults there instead as part of the implementation

// generate default constructor that will set these
struct state_t {
  decode_type_t vendor = decode_type_t::UNKNOWN;
  int16_t model = -1;
  bool power = false;
  opmode_t = opmode_t::kOff;
  ... etc ...
};

// designated init, just name things as they are
state_t state {
  .vendor = decode_type_t::Blah,
  .model = my_ac_model_const;
};

// state.power is still false by default

initState Method way may also work, but now we have to track both method and the struct. Plus, there are a lot of fields, and it is harder to track at a glance without using some kind of editor extension that would show the function signature?

---

Looking at the GCC manual, -Wmissing-field-initializers:
https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wmissing-field-initializers

Warn if a structure’s initializer has some fields missing. For example, the following code causes such a warning, because x.h is implicitly zero:

struct s { int f, g, h; };
struct s x = { 3, 4 };

Likewise, in C++ this option does not warn about the empty { } initializer, for example:

struct s { int f, g, h; };
s x = { };

I am able to see the issue with the GCC 4.8.2 that is used by the 2.7.4 esp8266 Core, so it seems just a mis-interpretation of the C vs. C++ rules that is fixed in the later versions of the compiler. Core 3.0.2 uses GCC 10.3 (CI examples build), and I am running GCC 11.2.1 locally (why I haven't seen the issue when trying to run tests)

~/.platformio/packages/[email protected]/bin/xtensa-lx106-elf-g++ -std=c++11 -Wall -Wextra -c -o state state.cpp
state.cpp: In function 'int main()':
state.cpp:78:19: warning: missing initializer for member 'state_t::protocol' [-Wmissing-field-initializers]
     state_t state{};
                   ^
state.cpp:78:19: warning: missing initializer for member 'state_t::model' [-Wmissing-field-initializers]
state.cpp:78:19: warning: missing initializer for member 'state_t::power' [-Wmissing-field-initializers]
state.cpp:78:19: warning: missing initializer for member 'state_t::mode' [-Wmissing-field-initializers]
state.cpp:78:19: warning: missing initializer for member 'state_t::degrees' [-Wmissing-field-initializers]
state.cpp:78:19: warning: missing initializer for member 'state_t::celsius' [-Wmissing-field-initializers]
state.cpp:78:19: warning: missing initializer for member 'state_t::fanspeed' [-Wmissing-field-initializers]
state.cpp:78:19: warning: missing initializer for member 'state_t::swingv' [-Wmissing-field-initializers]
state.cpp:78:19: warning: missing initializer for member 'state_t::swingh' [-Wmissing-field-initializers]
state.cpp:78:19: warning: missing initializer for member 'state_t::quiet' [-Wmissing-field-initializers]
state.cpp:78:19: warning: missing initializer for member 'state_t::turbo' [-Wmissing-field-initializers]
state.cpp:78:19: warning: missing initializer for member 'state_t::econo' [-Wmissing-field-initializers]
state.cpp:78:19: warning: missing initializer for member 'state_t::light' [-Wmissing-field-initializers]
state.cpp:78:19: warning: missing initializer for member 'state_t::filter' [-Wmissing-field-initializers]
state.cpp:78:19: warning: missing initializer for member 'state_t::clean' [-Wmissing-field-initializers]
state.cpp:78:19: warning: missing initializer for member 'state_t::beep' [-Wmissing-field-initializers]
state.cpp:78:19: warning: missing initializer for member 'state_t::sleep' [-Wmissing-field-initializers]
state.cpp:78:19: warning: missing initializer for member 'state_t::clock' [-Wmissing-field-initializers]

[crankyoldgit]

Honestly, picking the best C++ approach/idiom is beyond my skill level. E.g. this project was my first C++ code. Heck, my C experience was K&R; ANSI C had only recently come out when I stopped programming C.
So I'm happy to take advice here.

My preference is that all state_ts get initialised, not just zeroed before use. I.e. code safety & readability before pretty much everything else., Then what ever reduces code/flash/ram usage size. Of course, there are tradeoffs and speed to consider etc etc.
I.e. I'm happy to use some flash space etc to reduce O() from exponential to polynomial or linear etc.

[crankyoldgit]

Ref this and #1705, we should add a warning disable for that line, if we all agree the warning is incorrect, which I think we are. e.g.

#pragma GCC diagnostic ignored "-Wmissing-field-initializers"
stdAc::state_t result{};
#pragma GCC diagnostic pop

[mcspr]

I just noticed that replacing typedef struct { ... } name with struct name { ... }; removes the warning. I think we want to replace those instead of the pragma

Regarding init, my personal preference would be to just have the default-generated constructor and assign type member = default_member_value / type member { default_member_value }; (option 2 above)
Every state_t members will have default values, and we still could do designated init / object.value = 12345; where needed. afaik there's no overhead when doing assignments that way, since the ctor is also inline when it is in the header

edit:

Honestly, picking the best C++ approach/idiom is beyond my skill level. E.g. this project was my first C++ code. Heck, my C experience was K&R; ANSI C had only recently come out when I stopped programming C.
So I'm happy to take advice here.

This is a wonderful "set of guidelines for using C++ well. "
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines
(...there's also a point or two about default constructors and members btw, where I think I got the idea of doing this)

[henrygab]

I just noticed that replacing typedef struct { ... } name with struct name { ... }; removes the warning. I think we want to replace those instead of the pragma

Awesome! See https://godbolt.org/z/7d9P5aMGx.

This is the only change that can really be made without fundamentally changing some properties of the struct. I was unable to find any change by removing the typedef, however, which makes sense since classes and structs are equivalent in C++.

If there is agreement, I can further update the PR to have this change instead?

[crankyoldgit]

I'm happy as long as we are not adding bloat in the executable. As I said, this is above my experience level, so I'm happy to go with which ever option you two are happy with.

[mcspr]

If there is agreement, I can further update the PR to have this change instead?

Yes, please do
(timezones & mobile-internet be damned :)

I could take a look at the ctors since those are a whole separate thing, but only around ~monday or so. typedef struct -> struct should be harmless though

[henrygab]

The ctors are not required. Keeping as a POD type is OK, imho.

PR is updated with removal of 'typedef'.

[crankyoldgit]

If there is agreement, I can further update the PR to have this change instead?

Yes, please do (timezones & mobile-internet be damned :)

I could take a look at the ctors since those are a whole separate thing, but only around ~monday or so. typedef struct -> struct should be harmless though

@mcspr Were you going to do something or should we just mark this issue as fixed for now?

[crankyoldgit]

FYI, the committed changes above have now been included in the new v2.8.1 release of the library.

[crankyoldgit]

Added default initialisation.

[crankyoldgit]

FYI, the changes mentioned above have now been included in the new v2.8.2 release of the library.