Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compile as C++ for windows builds. #227

Merged
merged 1 commit into from Nov 3, 2016
Merged

Compile as C++ for windows builds. #227

merged 1 commit into from Nov 3, 2016

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Nov 2, 2016

Compiling as C++ changes nothing semantically, but ensures that we don't
break C++ compatibility while also retaining C compatibility.

C++ compatibility is useful for tooling and additional diagnostics and
analyses.


This change is Reviewable

@nurupo
Copy link
Member

nurupo commented Nov 2, 2016

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


toxcore/network.c, line 173 at r1 (raw file):

#if defined(__MACH__)
    int set = 1;
    return (setsockopt(sock, SOL_SOCKET, SO_NOSIGPIPE, (const char *)&set, sizeof(int)) == 0);

The fourth argument of setsockopt is of const void * type https://linux.die.net/man/2/setsockopt. Why use cast it to const char *?


toxcore/network.c, line 199 at r1 (raw file):

    int ipv6only = 0;
    socklen_t optsize = sizeof(ipv6only);
    int res = getsockopt(sock, IPPROTO_IPV6, IPV6_V6ONLY, (char *)&ipv6only, &optsize);

The fourth argument of getsockoptis of void * type https://linux.die.net/man/2/setsockopt. Why cast it to char *?


Comments from Reviewable

@iphydf
Copy link
Member Author

iphydf commented Nov 2, 2016

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


toxcore/network.c, line 173 at r1 (raw file):

Previously, nurupo wrote…

The fourth argument of setsockopt is of const void * type https://linux.die.net/man/2/setsockopt. Why use cast it to const char *?

Compatibility with the Windows version of `setsockopt`: https://msdn.microsoft.com/en-us/library/windows/desktop/ms740476(v=vs.85).aspx

toxcore/network.c, line 199 at r1 (raw file):

Previously, nurupo wrote…

The fourth argument of getsockoptis of void * type https://linux.die.net/man/2/setsockopt. Why cast it to char *?

Compatibility with the Windows version of `getsockopt`: https://msdn.microsoft.com/en-us/library/windows/desktop/ms738544(v=vs.85).aspx

Comments from Reviewable

@iphydf iphydf force-pushed the cxx-osx branch 6 times, most recently from a4bb15a to b0f7480 Compare November 2, 2016 18:44
Compiling as C++ changes nothing semantically, but ensures that we don't
break C++ compatibility while also retaining C compatibility.

C++ compatibility is useful for tooling and additional diagnostics and
analyses.
@nurupo
Copy link
Member

nurupo commented Nov 2, 2016

Reviewed 7 of 12 files at r2.
Review status: 9 of 14 files reviewed at latest revision, 2 unresolved discussions.


toxcore/network.c, line 173 at r1 (raw file):

Previously, iphydf wrote…

Compatibility with the Windows version of setsockopt: https://msdn.microsoft.com/en-us/library/windows/desktop/ms740476(v=vs.85).aspx

Wouldn't this remove the type warning from Windows build of toxcore and introduce the type warning on the Linux build of toxcore?

Also, it looks like you have undone your changes for this file in the PR.


Comments from Reviewable

@iphydf
Copy link
Member Author

iphydf commented Nov 2, 2016

Review status: 9 of 14 files reviewed at latest revision, 2 unresolved discussions.


toxcore/network.c, line 173 at r1 (raw file):

Previously, nurupo wrote…

Wouldn't this remove the type warning from Windows build of toxcore and introduce the type warning on the Linux build of toxcore?

Also, it looks like you have undone your changes for this file in the PR.

No, because `T*` is implicitly convertible to `void*`. If windows and linux didn't agree about the signedness of their pointer arguments, that would be a problem, but linux uses `void*`, so casting something to `char*` is fine.

Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Nov 2, 2016

Review status: 9 of 14 files reviewed at latest revision, 2 unresolved discussions.


toxcore/network.c, line 173 at r1 (raw file):

Previously, iphydf wrote…

No, because T* is implicitly convertible to void*. If windows and linux didn't agree about the signedness of their pointer arguments, that would be a problem, but linux uses void*, so casting something to char* is fine.

Isn't `void*` also implicitly convertible to `T*`, or it's just in C, not in C++? For example, you generally don't cast the return value of malloc in C code, but you did that in the LAN_discovery.c in this PR.

Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Nov 2, 2016

Review status: 9 of 14 files reviewed at latest revision, 2 unresolved discussions.


toxcore/network.c, line 173 at r1 (raw file):

Previously, nurupo wrote…

Isn't void* also implicitly convertible to T*, or it's just in C, not in C++? For example, you generally don't cast the return value of malloc in C code, but you did that in the LAN_discovery.c in this PR.

(Also, it looks like Reviewable has found network.c file this time. After your first comment the network.c file suddenly disappeared from Reviewable. Well, there was still a box for the file in Reviewable, but it had only the the comment threads, there were no lines of code in it, and it was saying something along the lines of this file not being in PR. Weird.)

Comments from Reviewable

@iphydf
Copy link
Member Author

iphydf commented Nov 2, 2016

Review status: 9 of 14 files reviewed at latest revision, 2 unresolved discussions.


toxcore/network.c, line 173 at r1 (raw file):

Previously, nurupo wrote…

(Also, it looks like Reviewable has found network.c file this time. After your first comment the network.c file suddenly disappeared from Reviewable. Well, there was still a box for the file in Reviewable, but it had only the the comment threads, there were no lines of code in it, and it was saying something along the lines of this file not being in PR. Weird.)

In C++, that implicit conversion is not valid. In C, you don't cast malloc, because in C you can have implicit declarations. In C++, you can't, so the reason not to cast malloc goes away (so we do cast malloc).

Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Nov 2, 2016

toxcore/network.c, line 173 at r1 (raw file):

Previously, iphydf wrote…

In C++, that implicit conversion is not valid. In C, you don't cast malloc, because in C you can have implicit declarations. In C++, you can't, so the reason not to cast malloc goes away (so we do cast malloc).

Ok.

(Oh, it was "File not in pull request at selected revision.", i.e. it was just me not using Reviewable correctly.)


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Nov 2, 2016

:lgtm_strong:


Reviewed 5 of 12 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@iphydf iphydf added this to the v0.0.3 milestone Nov 2, 2016
@nurupo
Copy link
Member

nurupo commented Nov 2, 2016

Nice branch name btw, totally Windows :P

@iphydf iphydf merged commit 96c672a into TokTok:master Nov 3, 2016
@iphydf iphydf deleted the cxx-osx branch November 3, 2016 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants