warning removal

[dansgithubuser]

I like having 0 warnings in a build....

I'm pretty confident with the _CRT_SECURE_NO_WARNINGS guard, which allows for specification of that directive outside, without creating a warning. I'm not sure if the second change can be more elegant.

Tested on Windows and Mac.

[rxi]

I agree completely with the intent -- ideally we have no warnings at least on the most popular platforms.

I don't currently have a windows box to test the _CRT_SECURE_NO_WARNINGS change on, but it looks sensible, so no problem there.

In regard to the casts for the timeval struct, the fields differ between platforms -- the change you made is the correct cast for BSD and, I assume, OSX, but on linux both fields are type long. Which platforms did you get warnings on for this bit of code? If they only occurred on windows we could remove the casts in the #ifdef's #else case, otherwise we're left with having to cast differently for each platform which seems a little excessive. If OSX does warn without a cast it might be best to make the #else case with no cast and add in a condition for OSX -- on my linux system, at least, both clang and gcc don't warn when there is no cast.

[fatcerberus]

Note that MSVC throws at least 6 warnings when compiling this for x64, due to conversion between SOCKET (which is a typedef for uint_ptr) and int.

There's also an issue that MSVC maps WSAAddressToString to the wide (Unicode) version, causing crashes. Fixing it requires calling the 'A' variant explicitly: WSAAddressToStringA()

[dansgithubuser]

Hey. I thought I'd get to this sooner, but I haven't so I'm going to at least note here that your comments are well-received and I'll update the request at some point.

[dansgithubuser]

OK, updated.

Regarding implicit casting, I simply disabled the MSVC warning for that code. I think this is the best match for the concerns raised.

For x64, I couldn't reproduce the warnings. This confuses me, because I know I've seen the WSAAddressToString before. For that one, I know the build description can set a preprocessor definition to get rid of the warning. Not sure what to do about the SOCKET/int casting because I haven't come across it.

I think this is OK. This PR is not designed to eliminate all warnings. It's an ongoing effort. The warnings addressed here were troublesome to me, so I got rid of them. I think it's a step in the right direction.

[fatcerberus]

SOCKET is an intptr_t on Windows, so you will only get the cast warning on an x64 build. I'm using MSVC 2013 for what it's worth. Maybe earlier versions didn't warn for this? I know I'm not using a particularly high warning level (/W3)

The WSAAddressToString issue is easily fixed by explicitly calling WSAaddressToStringA(). If not, and UNICODE is defined (which MSVC does by default), it won't just generate a warning, it will crash (buffer overflow due to writing a UTF-16 string to a char buffer).

[dansgithubuser]

Yes, I remember the WSAAddressToString issue better now. I think with the further updated change, now a build description can set a preprocessor define to remove a warning. And I think this is the smallest evil.

For whatever reason, I still don't get that warning I'm thinking of.

[rxi]

Hey, sorry for the slow response. I was hoping to be able to test some of the changes using MinGW under windows, but unfortunately my only windows computer has decided to no longer function so I can't test how these changes effect it.

Concerning the changes the ifdef here is redundant, as its already inside an ifdef block of the same case.

Regarding the other changes I don't see myself being able to test them in MSVC in the near future, nor do I know when I will have windows available such that I can test the changes with MinGW. I get the feeling that the pragmas may be an issue such that a check for MSVC (_MSC_VER) should be done rather than windows (_WIN32).

If you can test the changes with MinGW under windows and assure / change it such that there are no issues with -Wall -Wextra -std=c89 -pedantic I think I'd be happy merging the changes. As I don't see myself being able to test the code with MSVC myself it seems relying on others to assure compatibility with it might be the only real option.

[dansgithubuser]

Agreed about the ifdef. I'm not sure if _WIN32 means what it should mean, but I think this change is clearest about what dyad expects from its context.

Agreed about pragmas. I mistakenly inferred MSVC was the only Windows compiler dyad was interested in supporting.

My MSVC build is still warning free with this updated change. I ran python build.py daytime.c with MinGW in my path and it didn't print any warnings.

[rxi]

I'm not sure if _WIN32 means what it should mean

I believe it was put in for 32bit windows, but still remains in 64bit windows for backwards compatibility. As I understand it its the surest way (or at least the most common) to identify windows regardless of compiler.

All the changes look good. Thanks!