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

Exercises are not compiled with "warnings as errors" with MSVC (Microsoft Visual C++). #922

Closed
heijp06 opened this issue Nov 2, 2024 · 3 comments · Fixed by #923
Closed

Comments

@heijp06
Copy link
Contributor

heijp06 commented Nov 2, 2024

When compiling with CLang or GCC warnings are treated as errors. The relevant part of CMakeLists.txt is this:

if("${CMAKE_CXX_COMPILER_ID}" MATCHES "(GNU|Clang)")
    set_target_properties(${exercise} PROPERTIES
        COMPILE_FLAGS "-Wall -Wextra -Wpedantic -Werror"
    )
endif()

For MSVC no such option is set. As a consequence code that contains mistakes that the user should be guarded against (e.g. not returning a value in all code paths) compiles without errors. Also when uploading the code to the website the stricter rules on the server will make code that compiled locally fail there.

Setting warnings as errors for MSVC is not that hard. There is an MSVC specific part in CMakeLists.txt:

# Tell MSVC not to warn us about unchecked iterators in debug builds
if(${MSVC})
    set_target_properties(${exercise} PROPERTIES
        COMPILE_DEFINITIONS_DEBUG _SCL_SECURE_NO_WARNINGS)
endif()

This can be changed to:

# Tell MSVC not to warn us about unchecked iterators in debug builds
if(${MSVC})
    set_target_properties(${exercise} PROPERTIES
        COMPILE_DEFINITIONS_DEBUG _SCL_SECURE_NO_WARNINGS
        COMPILE_FLAGS "/WX")
endif()

Compiling with /WX will make MSVC treat warnings as errors.

If this change is deemed useful, I can pick it up.


As an aside, the GCC/CLang COMPILE_FLAGS: -Wall -Wextra -Wpedantic also raise the warning level of the compiler.

This is also possible for MSVC. The default warning level for MSVC is /W3, this can be set to /W4 or /Wall. I experimented with those levels and I do NOT propose setting those higher levels. E.g. with /WX /W4 the following code:

char capital_a = std::toupper('a');

Will fail to compile because the compiler warns (as error) about a possible loss of precision when setting capital_a (a char) to the return value of toupper() (an int).

Copy link
Contributor

github-actions bot commented Nov 2, 2024

Hello. Thanks for opening an issue on Exercism 🙂

At Exercism we use our Community Forum, not GitHub issues, as the primary place for discussion. That allows maintainers and contributors from across Exercism's ecosystem to discuss your problems/ideas/suggestions without them having to subscribe to hundreds of repositories.

This issue will be automatically closed. Please use this link to copy your GitHub Issue into a new topic on the forum, where we look forward to chatting with you!

If you're interested in learning more about this auto-responder, please read this blog post.

@github-actions github-actions bot closed this as completed Nov 2, 2024
@heijp06
Copy link
Contributor Author

heijp06 commented Nov 2, 2024

@vaeng I opened this issue, but it was auto-closed.

I am not really sure how to navigate this. I do not want to spam the forum, so this seemed more appropriate. Maybe I should also stop spamming here 🙂, let me know what you think.

I can pick up the issue if you think it is worthwhile. The change I propose to make is one that I consistently make to CMakeLists.txt when solving exercises on Windows.

@vaeng vaeng reopened this Nov 2, 2024
@vaeng
Copy link
Contributor

vaeng commented Nov 2, 2024

No need to post to the forums, if you ping me here. The forums are pretty quiet for CPP, so I don't mind the post there either. Do what feels best for you

In general I really appreciate the work you do for Windows, as it is often overlooked by our regulars, but heavily used by beginners.

Please keep in mind that I'm currently pretty busy and the other maintainers are somewhat inactive at the moment.

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 a pull request may close this issue.

2 participants