Rewrite IPAddress without union #8829
Conversation
- Using a union allowed undefined behavior for array-like INPUT and OUTPUT arguments through a 32-bit integer (and vice versa). C++ standard says (https://timsong-cpp.github.io/cppwp/n4659/class.union#1): "At most one of the non-static data members of an object of union type can be active at any time, that is, the value of at most one of the non-static data members can be stored in a union at any time. [ Note: One special guarantee is made in order to simplify the use of unions: If a standard-layout union contains several standard-layout structs that share a common initial sequence, and if a non-static data member of an object of this standard-layout union type is active and is one of the standard-layout structs, it is permitted to inspect the common initial sequence of any of the standard-layout struct members; see [class.mem]. — end note ]" and (https://timsong-cpp.github.io/cppwp/n4659/class.mem#22): "Two standard-layout unions are layout-compatible if they have the same number of non-static data members and corresponding non-static data members (in any order) have layout-compatible types." you can't hope that compilers don't seem to do undefined behavior at the moment
| uint8_t bytes[4]; // IPv4 address | ||
| uint32_t dword; | ||
| } _address; | ||
| alignas(alignof(uint32_t)) std::array<uint8_t,4> _address{}; // IPv4 address |
There was a problem hiding this comment.
Does std::array initialize with a zero value?
You did remove the default constructor, so it must be set to zero here.
Edit:
Yep, the {} does indeed initialize all elements.
See: https://en.cppreference.com/w/cpp/container/array
I was a bit worried it might only initialize the array and not just its elements as that was indeed an issue with C++11 requiring double braces.
|
Before we can do anything here, you need to first propose this upstream: https://github.com/arduino/ArduinoCore-API/blob/master/api/IPAddress.cpp |
Using a union allowed undefined behavior for 8-bit integer INPUT and OUTPUT arguments through a 32-bit integer (and vice versa). Write throught union::uint8_t[16] and read thoutght union::uint32_t[4] is a undefined behavior. C++ standard says (https://timsong-cpp.github.io/cppwp/n4659/class.union#1): "At most one of the non-static data members of an object of union type can be active at any time, that is, the value of at most one of the non-static data members can be stored in a union at any time. [ Note: One special guarantee is made in order to simplify the use of unions: If a standard-layout union contains several standard-layout structs that share a common initial sequence, and if a non-static data member of an object of this standard-layout union type is active and is one of the standard-layout structs, it is permitted to inspect the common initial sequence of any of the standard-layout struct members; see [class.mem]. — end note ]" and (https://timsong-cpp.github.io/cppwp/n4659/class.mem#22): "Two standard-layout unions are layout-compatible if they have the same number of non-static data members and corresponding non-static data members (in any order) have layout-compatible types." you can't hope that compilers don't seem to do undefined behavior at the moment Also created espressif/arduino-esp32#8829 PR dependent on changing the current API.
|
|
Thanks! Keep us posted :) |
|
@VojtechBartoska removed the milestone, because this is pending being merged upstream into the official Arduino API first. We will sync with what they currently have for 3.0.0 and later merge this, once updated upstream. |
They said that no std::* objects can be used to implement internal components. |
|
@safocl you will update this PR, once merged upstream, correct? |
yes, i will... |
|
because we are clearing PRs. I will close this one for now and @safocl can reopen it when main Arduino merges the proposal |
Rewrite IPAddress without union
Using a union allowed undefined behavior for array-like INPUT and OUTPUT arguments through a 32-bit integer (and vice versa).
C++ standard says (https://timsong-cpp.github.io/cppwp/n4659/class.union#1):
"At most one of the non-static data members of an object of union type
can be active at any time, that is, the value of at most one of the
non-static data members can be stored in a union at any time. [ Note:
One special guarantee is made in order to simplify the use of unions:
If a standard-layout union contains several standard-layout structs
that share a common initial sequence, and if a non-static data member
of an object of this standard-layout union type is active and is one
of the standard-layout structs, it is permitted to inspect the common
initial sequence of any of the standard-layout struct members; see
[class.mem]. — end note ]"
and (https://timsong-cpp.github.io/cppwp/n4659/class.mem#22):
"Two standard-layout unions are layout-compatible if they have the same number of non-static data members and corresponding non-static data members (in any order) have layout-compatible types."
you can't hope that compilers don't seem to do undefined behavior at the moment
note: this PR is recreated #8819 PR -- to fix #8819 (comment)