Skip to content

sys: net: sock: add uint32_t ipv4 address to address union#6114

Merged
miri64 merged 1 commit intoRIOT-OS:masterfrom
kaspar030:sock_uint32_ipv4
Nov 13, 2016
Merged

sys: net: sock: add uint32_t ipv4 address to address union#6114
miri64 merged 1 commit intoRIOT-OS:masterfrom
kaspar030:sock_uint32_ipv4

Conversation

@kaspar030
Copy link
Copy Markdown
Contributor

#5945 changed the ipv4 address type to an array (as it is for ipv6).

But "What we gain by introducing an uint32_t version of the IPv4 address is the ability to do 32bit register compares due to 4b alignment, which, apart from being easier to code (and read), is probably way more efficient than having to do memcpys and memcmps."

@kaspar030 kaspar030 added the Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation label Nov 13, 2016
#endif
uint8_t ipv4[4]; /**< IPv4 address mode */
uint8_t ipv4[4]; /**< IPv4 address mode */
uint32_t ipv4_u32[4]; /**< IPv4 address in network byte order */
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can I haz emphasis on network byte order?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? It's not emphasized for the port either.

#endif
uint8_t ipv4[4]; /**< IPv4 address mode */
uint8_t ipv4[4]; /**< IPv4 address mode */
uint32_t ipv4_u32[4]; /**< IPv4 address in network byte order */
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, too.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Nov 13, 2016

Please squash.

@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 13, 2016
#endif
uint8_t ipv4[4]; /**< IPv4 address mode */
uint8_t ipv4[4]; /**< IPv4 address mode */
uint32_t ipv4_u32[4]; /**< IPv4 address *in network byte order* */
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just wondering why [4]? Didn't you mean uint32_t ipv4_u32;?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, sorry for overlooking that

miri64
miri64 previously requested changes Nov 13, 2016
#endif
uint8_t ipv4[4]; /**< IPv4 address mode */
uint8_t ipv4[4]; /**< IPv4 address mode */
uint32_t ipv4_u32[4]; /**< IPv4 address *in network byte order* */
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, sorry for overlooking that

@kaspar030
Copy link
Copy Markdown
Contributor Author

Yeah, only one uint32_t... fixed & immediately squashed.

@cgundogan cgundogan dismissed miri64’s stale review November 13, 2016 11:33

changes were addressed

Copy link
Copy Markdown
Member

@cgundogan cgundogan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

@miri64 miri64 merged commit e4b3331 into RIOT-OS:master Nov 13, 2016
@kaspar030 kaspar030 deleted the sock_uint32_ipv4 branch November 13, 2016 13:21
#endif
uint8_t ipv4[4]; /**< IPv4 address mode */
uint8_t ipv4[4]; /**< IPv4 address mode */
uint32_t ipv4_u32; /**< IPv4 address *in network byte order* */
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not network_uint32_t?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using sock on Linux [1], which doesn't have byteorder.h.

[1] https://github.com/kaspar030/sock

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants