Skip to content

suppress msvc warnings about totally safe functions #298

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

Merged
merged 2 commits into from
Jun 27, 2023
Merged

suppress msvc warnings about totally safe functions #298

merged 2 commits into from
Jun 27, 2023

Conversation

diamante0018
Copy link
Contributor

Compiling with MSVC proves to be an unpleasant experience due to MSVC unreasonably complaining about the following:
It complains about this library using fopen instead of their CRT non-standard fopen_s function.
This warning cannot be suppressed by adding a definition to the simplecpp.h header, it does not work somehow.
It is a bug I've always had with programs compiled with MSVC. I have always added _CRT_SECURE_NO_WARNINGS to the preached header but because this repository does not have one I have opted to add it to the CMAKE file.

Finally, the most annoying warnings are suppressed successfully using pragma directives.
Without these pragmas, my MSVC produces an immense build log full of warnings.
I hope this PR is appreciated in respect of: #288 (comment)

Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

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

Sounds good to me. I am no cmake expert so I can't say anything about that change.

Adding such pragmas in the header is fine for me.

But if ci is happy I think we can merge this.

@diamante0018
Copy link
Contributor Author

CI is all green first try? Impossible 😺

@danmar danmar merged commit a60f133 into danmar:master Jun 27, 2023
@firewave
Copy link
Collaborator

firewave commented Aug 10, 2023

"totally safe" is an overstatement. The warnings definitely indicate issues but fixing these kind of warnings has been a sore spot for a while so let's not get into this.

@@ -15,6 +15,8 @@ endfunction()

if (CMAKE_CXX_COMPILER_ID MATCHES "GNU")
add_compile_options(-Wall -Wextra -pedantic -Wcast-qual -Wfloat-equal -Wmissing-declarations -Wmissing-format-attribute -Wredundant-decls -Wshadow -Wundef -Wold-style-cast -Wno-multichar)
elseif (CMAKE_CXX_COMPILER_ID MATCHES "MSVC")
add_compile_definitions(_CRT_SECURE_NO_WARNINGS)
Copy link
Collaborator

Choose a reason for hiding this comment

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

These warnings should be "fixed" by specifying _CRT_SECURE_CPP_OVERLOAD_STANDARD_NAMES=1 instead - see https://learn.microsoft.com/en-us/cpp/c-runtime-library/secure-template-overloads.

I wasn't aware of this so it might have been introduced only in recent years.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not so sure about this, it seems to be the usual MSVC scaring people into using _s functions when the 'normal' C standard functions can be used just fine if used correctly. _CRT_SECURE_NO_WARNINGS is just the broader macro that tells MSVC to stop complaining.

@@ -44,6 +44,12 @@
#define nullptr NULL
#endif

#if defined(_MSC_VER)
// suppress warnings about "conversion from 'type1' to 'type2', possible loss of data"
# pragma warning(disable : 4267)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is really, really, really, REALLY bad as it will disable the warnings for every file which includes this. Use add_compile_options() with the /Wd Visual Studio options instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah you are right I should have used pragma push(warning) and pop surrounding this header file but I forgot

Copy link
Collaborator

Choose a reason for hiding this comment

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

I fixed those in #306.

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.

3 participants