Skip to content
New issue

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

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

Already on GitHub? # to your account

[C] Windows fixes, next part #863

Closed
wants to merge 7 commits into from

Conversation

ltrzesniewski
Copy link
Contributor

These are larger fixes than in my last PR, I recommend reviewing this commit-by-commit.

The remaining warnings that MSVC outputs are:

  • Various assignments of size_t values to int (I'm surprised GCC doesn't warn about those)
  • getsockopt and setsockopt have a char* type for the value instead of void*. It would be nice to have an aeron_* version of those to deal with those warnings if you're OK with that.

Also, errors aren't currently logged properly on Windows (strerror is kind of useless and errno isn't used by winsock). Fixing that would require replacing calls to aeron_set_err that use strerror with something similar to what aeron_set_windows_error does. I'm not yet sure about the best way to do it, but would you agree to such a change in principle?

 - A function with the wrong signature was being passed to CreateThread
 - The thread handle was never closed
 - Failure to start a thread was not detected
 - Lock/unlock did not work as an indirection was missing
 - Made the function prototypes match the Linux version
 - Added aeron_mutex_destroy
 - The SOCKET type was incorrectly cast to int, even though it's larger
 - Renamed aeron_fd_t to aeron_socket_t as a socket handle is not a file descriptor on Windows
 - Don't try to silently adjust the supplied length to the file size
 - off_t is 32 bits, so offset >> 32 generates a warning
@mjpt777
Copy link
Contributor

mjpt777 commented Feb 20, 2020

It would be better to limit the scope of a PR. For example separate out addressing warnings from behavioural change. It is easier to discuss behaviour change in the context of each change.

@ltrzesniewski
Copy link
Contributor Author

Yes, I understand. I didn't want to flood you with PRs, but I can split this into 6 or 7 PRs if you prefer.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants