Skip to content

net/sock: include net/af.h#6137

Closed
haukepetersen wants to merge 1 commit intoRIOT-OS:masterfrom
haukepetersen:opt_sock_incafheader
Closed

net/sock: include net/af.h#6137
haukepetersen wants to merge 1 commit intoRIOT-OS:masterfrom
haukepetersen:opt_sock_incafheader

Conversation

@haukepetersen
Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen commented Nov 17, 2016

When using sock, it is very very probable, that one needs to define the used family (e.g. AF_INET6 or AF_INET). Including this header in sock.h 'packages' this header with sock, so that one does not need to explicitly include it when using sock.

@haukepetersen haukepetersen added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 17, 2016
@haukepetersen haukepetersen added this to the Release 2017.01 milestone Nov 17, 2016
@miri64
Copy link
Copy Markdown
Member

miri64 commented Nov 17, 2016

I think @kaspar030 has a far stronger opinion than me on that.

@miri64 miri64 removed their assignment Nov 17, 2016
@miri64
Copy link
Copy Markdown
Member

miri64 commented Nov 17, 2016

[net/af.h is not part of POSIX, jeopardizing sock's portability and including the POSIX equivalent <sys/socket.h> would require to add the POSIX include path (which I do not support because this might be causing most likely some issues when posix module is not included)].

miri64
miri64 reviewed Nov 17, 2016
View reviewed changes
sys/include/net/sock.h

#include <stdint.h>

#include "net/af.h"
Copy link
Copy Markdown
Member

@miri64 miri64 Nov 17, 2016

Choose a reason for hiding this comment

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

@kaspar030 @haukepetersen would you be okay with

#ifdef RIOT_VERSION
#include "net/af.h"
#else
#include <sys/socket.h>
#endif

?

Copy link
Copy Markdown
Contributor Author

@haukepetersen haukepetersen Nov 17, 2016

Choose a reason for hiding this comment

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

I would

@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Nov 18, 2016

sock.h includes sock_types.h, wouldn't that be the place to include whatever defines AF_*?

@miri64
Copy link
Copy Markdown
Member

miri64 commented Nov 18, 2016

sock.h includes sock_types.h, wouldn't that be the place to include whatever defines AF_*?

Maybe an even better (networking sub-layer independent) solution. Will fix for GNRC.

@miri64 miri64 mentioned this pull request Nov 18, 2016
Merged
@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Nov 18, 2016

@kaspar030: agree, makes sense. So I close this PR in favor of that solution.

@haukepetersen haukepetersen deleted the opt_sock_incafheader branch November 18, 2016 13:28
This was referenced Nov 18, 2016
Merged
Merged
Merged
@miri64 miri64 removed this from the Release 2017.01 milestone Nov 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miri64 miri64 miri64 left review comments

Assignees

@kaspar030 kaspar030

Labels

Area: network Area: Networking 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@haukepetersen @miri64 @kaspar030