Use direct member initialization instead of ctr initialisation #7558
Conversation
dirkmueller
force-pushed
the
no_ctr_initialization
branch
from
b8f2990 to
2385eeb
Compare
| #define SSDP_PORT 1900 | ||
| #define SSDP_METHOD_SIZE 10 | ||
| #define SSDP_URI_SIZE 2 | ||
| #define SSDP_BUFFER_SIZE 64 | ||
| #define SSDP_MULTICAST_TTL 2 |
There was a problem hiding this comment.
I would leave SSDP_INTERVAL and SSDP_MULTICAST_TTL defined and used in member initializers for clarity (maybe also adding the unity: SSDP_INTERVAL_SECONDS)
There was a problem hiding this comment.
that'd defeat the purpose of the PR to only have initialization once in one place. I can rename the _interval member variable to _interval_s to keep the unit.
this define is used only in one place, adding this #define does not add make things easier (you need to look it up anyway when you come across reading the constructor).
if thats a hard objection the only thing I'd do is then to remove ESP82666SSDP* completely from this cleanup, leaving it as it is now.
There was a problem hiding this comment.
What @d-a-v means is expose the defines, i.e.: move them to the header, maybe turn them into constexpr, and use the relevant ones in the direct member assignment instead of using literal numbers that don't convey meaning.
There was a problem hiding this comment.
BTW, if it is true that the new MDNS responder also handles SSDP, then this SSDP class will likely be pulled out.
|
Other than trading one style for another, @dirkmueller, it doesn't look like this has any actual effect on the generated code, no? I'm not really convinced one way is better than the other (and the whole codebase mixes and matches between the two ways in my experience)... |
|
@earlephilhower yes, generated code will remain identical. We could decide for one style (e.g the one proposed in this PR) and I can make the code base consistent. There are advantages (reordering class members doesn't require code changes in the constructor code, multiple constructors don't have to repeate initializations) |
|
Isn't there a difference here? But like I said, have to check in the standard to make sure that's what is happening. |
| #define SSDP_PORT 1900 | ||
| #define SSDP_METHOD_SIZE 10 | ||
| #define SSDP_URI_SIZE 2 | ||
| #define SSDP_BUFFER_SIZE 64 | ||
| #define SSDP_MULTICAST_TTL 2 |
There was a problem hiding this comment.
What @d-a-v means is expose the defines, i.e.: move them to the header, maybe turn them into constexpr, and use the relevant ones in the direct member assignment instead of using literal numbers that don't convey meaning.
| #define SSDP_PORT 1900 | ||
| #define SSDP_METHOD_SIZE 10 | ||
| #define SSDP_URI_SIZE 2 | ||
| #define SSDP_BUFFER_SIZE 64 | ||
| #define SSDP_MULTICAST_TTL 2 |
There was a problem hiding this comment.
BTW, if it is true that the new MDNS responder also handles SSDP, then this SSDP class will likely be pulled out.
No, its identical, see this FAQ: https://isocpp.org/wiki/faq/cpp11-language-classes#member-init you're right that initalizer lists (e.g. foo::foo() : member(0) {} ) allow avoiding default constructor invocation of the member (which optimizes runtime) as well as allow the compiler to be clever in initializing objects. however one or the other is just syntactical sugar that is making error-prone repetition of code obsolete (DRY - don't repeat yourself). |
dirkmueller
force-pushed
the
no_ctr_initialization
branch
from
9faeb8a to
65775e9
Compare
This removes a bit of code repetition.
dirkmueller
force-pushed
the
no_ctr_initialization
branch
from
10c57f7 to
ade47e2
Compare
* master: (37 commits) BREAKING: Change return EEPROM.end() to bool (esp8266#7630) BREAKING: Change return type of channel() (esp8266#7656) BREAKING: Change return type of RSSI() (esp8266#7657) Add Copyright notice to Schedule.h (esp8266#7653) MDNS MultiInterface (esp8266#7636) BREAKING: Add Wrong Password wifi status case (esp8266#7652) New flash writing method with offset/memory/size alignment handling (esp8266#7514) Fix error when debug enabled but no port chosen (esp8266#7648) LEAmDNSv2: change a macro name to be independant from LEAmDNS1 (esp8266#7640) Allow test framework to use cores/esp8266/Arduino.h directly (esp8266#7377) Update OTA HTTP Server Header Information (esp8266#7633) Add missing sntp_init/sntp_stop (esp8266#7628) Use direct member initialization instead of ctr initialisation (esp8266#7558) Prevent rewriting Updater_Signing.h if content unchanged (esp8266#7627) Add WiFi Multi to readme.rst Remove stray axtls refs, deprecated compat funcs (esp8266#7626) Pull deprecated axtls link (esp8266#7624) Redesign ESP8266WiFiMulti.[cpp|h] Update README.md (esp8266#7623) Eliminate code duplication by template for printNumber(...)/printFloat(...). ...
This removes a bit of code repetition.