-
Notifications
You must be signed in to change notification settings - Fork 6
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
Fix some miscellaneous MSVC warnings #7
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks awesome to me, just space changes. Thank you!
/* NOTE: This gives warnings on MSVC, so I removed this | ||
* for the oldest version of it I could test (19.20). */ | ||
/* TODO: Test with the other described compilers if this | ||
* is _really_ necessary. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* is _really_ necessary. */ | |
* is REALLY necessary. */ |
Not sure, just a suggestion, for me both work.
(void) cond; | ||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(void) cond; | |
return 0; | |
(void) cond; | |
return 0; |
/* MSVC sees strncpy() as insecure. */ | ||
#define _CRT_SECURE_NO_WARNINGS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/* MSVC sees strncpy() as insecure. */ | |
#define _CRT_SECURE_NO_WARNINGS | |
/* INFO: MSVC sees strncpy() as insecure. */ | |
#define _CRT_SECURE_NO_WARNINGS | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, is this widely compatible or affects files outside the project? For second case, I believe we can #undef it? Wouldn't be good to apply such for files outside.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid there's no easy way to do this.
We could of course undef it, but it would still propagate because of windows.h's include guards.
The best solution would be to not require windows.h in the header, but the only way to do this is making all structs (or parts of structs using specific types) opaque.
I think the best course of action is just to remove the define and endur the warning or add an option for the user to disable the define.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion we can should* strncpy for memcpy and just handle NULL terminator ending. With proper testing, it becomes safe anyway.
Changes
_CRT_SECURE_NO_WARNINGS
to fix a warning related to strncpy().(void) cond
incthreads_cond_destroy()
because it isn't used.Why
Because nobody likes compile-time warnings.
Checkmarks
Additional information