Rewrite IPAddress without union #8819
Conversation
Inside the function, this overload truncated the data type to a shorter one. This could break some users' hopes.
- 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."
|
|
|
Hello @safocl, please sign CLA. Thanks! |
I did it. |
|
Example with UB -- from cppreference |
|
Modifying TwoWire seems unrelated to this PR and may introduce breaking changes. |
yes, this is separate PR. |
| return *this; | ||
| } | ||
|
|
||
| IPAddress& IPAddress::operator=(uint32_t address) | ||
| { | ||
| _address.dword = address; | ||
| uint32_t& addressRef = reinterpret_cast<uint32_t&>(_address.front()); |
There was a problem hiding this comment.
is _address.front() guaranteed to be 32-bit aligned?
If not, then this will cause undefined behavior.
There was a problem hiding this comment.
is
_address.front()guaranteed to be 32-bit aligned? If not, then this will cause undefined behavior.
It is guaranteed be aligned as structure -- array is once member in this structure -- it has same address. It behavior same as union or c-style array (uint8_t a[4]).
Standart says (https://timsong-cpp.github.io/cppwp/n4659/array):
class template for storing fixed-size sequences of objects. An array is a contiguous container. An instance of array<T, N> stores N elements of type T, so that size() == N is an invariant.
An array is an aggregate that can be list-initialized with up to N elements whose types are convertible to T.
https://timsong-cpp.github.io/cppwp/n4659/dcl.init.aggr#1:
An aggregate is an array or a class with
(1.1) no user-provided, explicit, or inherited constructors ([class.ctor]),
(1.2) no private or protected non-static data members (Clause [class.access]),
(1.3) no virtual functions, and
(1.4) no virtual, private, or protected base classes ([class.mi]).
[ Note: Aggregate initialization does not allow accessing protected and private base class' members or constructors. — end note ]
https://timsong-cpp.github.io/cppwp/n4659/dcl.init.aggr#2:
2 The elements of an aggregate are:
(2.1) for an array, the array elements in increasing subscript order, or
(2.2) for a class, the direct base classes in declaration order, followed by the direct non-static data members ([class.mem]) that are not members of an anonymous union, in declaration order.
and (https://timsong-cpp.github.io/cppwp/n4659/container.requirements#general-6):
begin() returns an iterator referring to the first element in the container. end() returns an iterator which is the past-the-end value for the container. If the container is empty, then begin() == end().
https://timsong-cpp.github.io/cppwp/n4659/sequence.reqmts#tab:containers.sequence.optional:
a.front() = *a.begin()
It mean that &_address.front() == &(*_address.begin()) == &_address == &IPAddress
and correspondingly an align of _address.front() depend on an align of IPAddress structure.
There was a problem hiding this comment.
but this example shows that alignof = 1 for struct with std::array<uint8_t,4> member.
alignas(4) is required to achieve 32-bit alignment
struct alignas(4) Sarray {
std::array<uint8_t,4> a{};
};
There was a problem hiding this comment.
but this example shows that class IPAddress has 4 bytes of aligment because it inherits from an abstract type (with vtable).
in this case, the _address field still has 1 byte of alignment. -- need alignas( alignof(uint32_t) ) for IPAddress.
struct IPAddress: public Printable
{
alignas( alignof(uint32_t) ) std::array<uint8_t,4> _address{};
};
I didn’t initially take into account that inheritance occurs
There was a problem hiding this comment.
here, of course, I would like to generally highlight that uint32_t may not exist at all on a specific platform.
Maybe uint_least32_t should be used for compatibility?
There was a problem hiding this comment.
uint32_t is known in all platforms this repository covers.
I don't know uint_least32_t, but it seems to be not guaranteed to be a fixed length type, which will cause issues when it is used in some struct of fixed length or offset of members (e.g. storing a struct or transferring it to another node)
There was a problem hiding this comment.
I don't know
uint_least32_t, but it seems to be not guaranteed to be a fixed length type, which will cause issues when it is used in some struct of fixed length or offset of members (e.g. storing a struct or transferring it to another node)
https://timsong-cpp.github.io/cppwp/n4659/cstdint#syn -- types that are at least N-bit
| // Overloaded cast operator to allow IPAddress objects to be used where a pointer | ||
| // to a four-byte uint8_t array is expected | ||
| operator uint32_t() const | ||
| { | ||
| return _address.dword; | ||
| return reinterpret_cast<const uint32_t&>(_address.front()); |
There was a problem hiding this comment.
Idem, reinterpret_cast should be used with utmost care, especially with alignment.
But the Wire files are included in this PR. |
ok. I'll do a new PR. |
|
i recreated PR #8829 |
|
You could simply have removed the 2 Wire files from this PR by copying the currently existing code of the main branch and committing it to this PR. |
|
too many pull requests open. please fix them up and leave only minimal amount open (like one for Wire and one for IPAddress) |
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