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

Implement various Windows build fixes #14

Merged
merged 1 commit into from
Jul 31, 2024
Merged

Conversation

braewoods
Copy link
Contributor

This also closes #12.

@hhromic
Copy link
Owner

hhromic commented Jul 30, 2024

Hi @braewoods , thanks for the contribution and helping on the Windows front! 💯

Your changes looks good but I have the following two questions:

  • For setsockopt, according to POSIX and Windows the fourth argument should be qualified with const. Therefore, can you update your changes for (const void *) instead? Thanks for spotting this one!
  • The same above applies to sendto as well for the second argument.
  • Can you elaborate on why you need to switch the PRIu32 format to lu? I just tested a simple program in MSYS2 (which I also use) and it works fine for the output of ntohl. The PRIu32 format macro is supposed to be portable.

@braewoods
Copy link
Contributor Author

* For `setsockopt`, according to [POSIX](https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/functions/setsockopt.html) and [Windows](https://learn.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-setsockopt) the fourth argument should be qualified with `const`.  Therefore, can you update your changes for `(const void *)` instead? Thanks for spotting this one!
* The same above applies to `sendto` as well for the second argument.

Fair, but I didn't think it would matter in practice because non-const variables will get silently promoted to const in such contexts.

* Can you elaborate on why you need to switch the `PRIu32` format to `lu`? I just tested a simple program in MSYS2 (which I also use) and it works fine for the output of `ntohl`. The `PRIu32` format macro is supposed to be portable.

I was getting compiler warnings on the UCRT64 toolchain because ntohl returns an unsigned long, where as PRIu32 resolves to a printf specifier for unsigned int. In practice this should not matter for Windows because long and int are the same size even on 64 bit Windows, but this is the ABI Microsoft decided on, and I have no idea why GCC is even warning for it.

@braewoods
Copy link
Contributor Author

First set of changes done.

@braewoods
Copy link
Contributor Author

Here's the errors I was getting without my changes:

src/e131.c:316:19: error: format '%u' expects argument of type 'unsigned int', but argument 3 has type 'u_long' {aka 'long unsigned int'} [-Werror=format=]
  316 |   fprintf(stream, "  Layer Vector ........... %" PRIu32 "\n", ntohl(packet->root.vector));
      |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~              ~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                                                               |
      |                                                               u_long {aka long unsigned int}
In file included from src/e131.c:26:
C:/msys64/ucrt64/include/inttypes.h:101:17: note: format string is defined here
  101 | #define PRIu32 "u"
src/e131.c:327:19: error: format '%u' expects argument of type 'unsigned int', but argument 3 has type 'u_long' {aka 'long unsigned int'} [-Werror=format=]
  327 |   fprintf(stream, "  Layer Vector ........... %" PRIu32 "\n", ntohl(packet->frame.vector));
      |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~              ~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                                                               |
      |                                                               u_long {aka long unsigned int}
C:/msys64/ucrt64/include/inttypes.h:101:17: note: format string is defined here
  101 | #define PRIu32 "u"```

@hhromic
Copy link
Owner

hhromic commented Jul 31, 2024

Fair, but I didn't think it would matter in practice because non-const variables will get silently promoted to const in such contexts.

Yep, I'm aware that promotion takes place, but I prefer code to be as correct as possible, thanks for updating it!

I was getting compiler warnings on the UCRT64 toolchain because ntohl returns an unsigned long, where as PRIu32 resolves to a printf specifier for unsigned int. In practice this should not matter for Windows because long and int are the same size even on 64 bit Windows, but this is the ABI Microsoft decided on, and I have no idea why GCC is even warning for it.

Yes, I went again to check my toy example where I was testing this and I made a silly mistake. I now got the same warning you were getting and understand why too as you explained.

However, instead of #ifdef pre-processor directives, I would prefer to do a casting instead:

fprintf(stream, "  Layer Vector ........... %" PRIu32 "\n", (uint32_t) ntohl(packet->root.vector));

How does that sound to you?

@braewoods
Copy link
Contributor Author

Fine by me. It's been changed. I just felt like the macro one was better at explaining why the code was modified for future readers, due to the WIN32 macro check.

@hhromic
Copy link
Owner

hhromic commented Jul 31, 2024

Fine by me. It's been changed. I just felt like the macro one was better at explaining why the code was modified for future readers, due to the WIN32 macro check.

Mmh yeah I think that you are right. The macro check was indeed better at explaining.
Apologies, can you revert it then to what you had before? I will approve and merge afterwards 👍.

@braewoods
Copy link
Contributor Author

That should do it.

Copy link
Owner

@hhromic hhromic left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

@hhromic hhromic merged commit 5dedd42 into hhromic:master Jul 31, 2024
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.

win compilation errors
2 participants