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

Fix some miscellaneous MSVC warnings #7

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

0xfadead
Copy link
Contributor

Changes

  • Added definition _CRT_SECURE_NO_WARNINGS to fix a warning related to strncpy().
  • Excluded newer versions of MSVC on (old) line 139.
  • Added (void) cond in cthreads_cond_destroy() because it isn't used.

Why

Because nobody likes compile-time warnings.

Checkmarks

  • The modified functions have been tested. (MSVC 19.20)
  • Used the same indentation as the rest of the project.

Additional information

Copy link
Member

@ThePedroo ThePedroo left a 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. */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* is _really_ necessary. */
* is REALLY necessary. */

Not sure, just a suggestion, for me both work.

Comment on lines +330 to 331
(void) cond;
return 0;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(void) cond;
return 0;
(void) cond;
return 0;

Comment on lines +10 to +11
/* MSVC sees strncpy() as insecure. */
#define _CRT_SECURE_NO_WARNINGS
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/* MSVC sees strncpy() as insecure. */
#define _CRT_SECURE_NO_WARNINGS
/* INFO: MSVC sees strncpy() as insecure. */
#define _CRT_SECURE_NO_WARNINGS

Copy link
Member

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.

Copy link
Contributor Author

@0xfadead 0xfadead Jan 21, 2025

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.

Copy link
Member

@ThePedroo ThePedroo Jan 21, 2025

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.

@ThePedroo ThePedroo added enhancement New feature or request confirmed This issue or pull request is confirmed to be done. labels Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed This issue or pull request is confirmed to be done. enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants