Better fix for #1699

[henrygab]

In PR #1700, I removed the curly braces, because GCC (ESP8266) warned about uninitialized fields (see #1699). After digging deeper, the warnings that GCC emitted were incorrect. Special shout out to @mcspr for questioning this, and making me dig deeper!

PR #1700 resulted in the removal of explicitly zero-initialization of the structure. While the function then initialized each field, there are likely unused packing bytes (e.g., between final beep and sleep) that would not be initialized to zero.

I admit that I'm not sure how GCC handles returning a local struct ... (does it use the copy constructor?), but regardless of whether this actually exposes uninitialized bytes, it's still bad practice(*) to not properly initialized structures.

@crankyoldgit ... please accept my apologies. I didn't dig deeply enough, and made a mistake.

[crankyoldgit]

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

[crankyoldgit]

@crankyoldgit ... please accept my apologies. I didn't dig deeply enough, and made a mistake.

No need to apologise at all.

[henrygab]

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

OK, I've added this much more targeted fix:

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

[crankyoldgit]

LGTM.
Thanks again.

[henrygab]

What is this code lint error? the code is identical to that around it? Is this a spurious error?

[crankyoldgit]

src/IRsend.h:97: Do not indent within a namespace [runtime/indentation_namespace] [4]
Done

[henrygab]

@crankyoldgit ... I have looked at this four different ways. main branch has the following:

IRremoteESP8266/src/IRsend.h

Lines 96 to 116 in 56d5352

	/// Structure to hold a common A/C state.
	typedef struct {
	decode_type_t protocol;
	int16_t model;
	bool power;
	stdAc::opmode_t mode;
	float degrees;
	bool celsius;
	stdAc::fanspeed_t fanspeed;
	stdAc::swingv_t swingv;
	stdAc::swingh_t swingh;
	bool quiet;
	bool turbo;
	bool econo;
	bool light;
	bool filter;
	bool clean;
	bool beep;
	int16_t sleep;
	int16_t clock;
	} state_t;

It's showing two space indentation before typedef, with and addition two space indentation for the fields.

Then there is the PR:

https://github.com/henrygab/IRremoteESP8266/blob/645ff329272397533664edb75a7991a8c02ceb33/src/IRsend.h#L96-L116

This is using the exact same spacing... the indentation has not changed.

Is this blocking the PR, unless it reformats the indentation of that entire namespace?

[crankyoldgit]

@crankyoldgit ... I have looked at this four different ways. ...

It's showing two space indentation before typedef, with and addition two space indentation for the fields.

Then there is the PR:

This is using the exact same spacing... the indentation has not changed.

Is this blocking the PR, unless it reformats the indentation of that entire namespace?

In short, yes.

@henrygab It appears we've triggered the cpplinter to do it's job correctly.
According to the examples in: https://google.github.io/styleguide/cppguide.html#Namespaces we shouldn't be using the initial two spaces in the indentation. i.e. No indentation for namespaces.

I'll adjust your branch/PR to address it.

[henrygab]

OK, thanks for adjusting the PR, and approving / merging. It just seemed (seems?) wrong.

It could be that the linter is smart ... e.g., maybe it won't report an error for a section of code unless it's being changed?

either way, many thanks!

[crankyoldgit]

OK, thanks for adjusting the PR, and approving / merging. It just seemed (seems?) wrong.

You're not alone. I had to dig into it to find out why it was triggering.

It could be that the linter is smart ... e.g., maybe it won't report an error for a section of code unless it's being changed?

No, it's not smart and it is standalone. It does the whole thing. It often misses stuff due to parsing issues.
e.g. casting experessions/variables. It sometimes allows (uint16_t)(foobar * 21) and sometimes requires the reinterpret_cast<uint16_t>(foobar * 21) style. So a slight change to the surrounding/nearby code can make it spring to life.

End of the day, it's a piece of python that does a pretty good (but not perfect) job, and is often correct.

either way, many thanks!

No worries.