exercism / cpp

Exercism exercises in C++.
https://exercism.org/tracks/cpp
MIT License
259 stars 214 forks source link

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

Closed heijp06 closed 1 week ago

heijp06 commented 1 week ago

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).

github-actions[bot] commented 1 week ago

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%0D%0A%20%20%20%20set_target_properties($%7Bexercise%7D%20PROPERTIES%0D%0A%20%20%20%20%20%20%20%20COMPILE_FLAGS%20%22-Wall%20-Wextra%20-Wpedantic%20-Werror%22%0D%0A%20%20%20%20)%0D%0Aendif()%0D%0A%60%60%60%0D%0A%0D%0AFor%20MSVC%20no%20such%20option%20is%20set.%20As%20a%20consequence%20code%20that%20contains%20mistakes%20that%20the%20user%20should%20be%20guarded%20against%20(e.g.%20not%20returning%20a%20value%20in%20all%20code%20paths)%20compiles%20without%20errors.%20Also%20when%20uploading%20the%20code%20to%20the%20website%20the%20stricter%20rules%20on%20the%20server%20will%20make%20code%20that%20compiled%20locally%20fail%20there.%0D%0A%0D%0ASetting%20warnings%20as%20errors%20for%20MSVC%20is%20not%20that%20hard.%20There%20is%20an%20%5BMSVC%20specific%20part%20in%20CMakeList.txt%5D(https://github.com/exercism/cpp/blob/3d106a5b8768c17c716fd0692013ffe4ae9937de/exercises/practice/hello-world/CMakeLists.txt#L57):%0D%0A%0D%0A%60%60%60cmake%0D%0A#%20Tell%20MSVC%20not%20to%20warn%20us%20about%20unchecked%20iterators%20in%20debug%20builds%0D%0Aif($%7BMSVC%7D)%0D%0A%20%20%20%20set_target_properties($%7Bexercise%7D%20PROPERTIES%0D%0A%20%20%20%20%20%20%20%20COMPILE_DEFINITIONS_DEBUG%20_SCL_SECURE_NO_WARNINGS)%0D%0Aendif()%0D%0A%60%60%60%0D%0A%0D%0AThis%20can%20be%20changed%20to:%0D%0A%0D%0A%60%60%60cmake%0D%0A#%20Tell%20MSVC%20not%20to%20warn%20us%20about%20unchecked%20iterators%20in%20debug%20builds%0D%0Aif($%7BMSVC%7D)%0D%0A%20%20%20%20set_target_properties($%7Bexercise%7D%20PROPERTIES%0D%0A%20%20%20%20%20%20%20%20COMPILE_DEFINITIONS_DEBUG%20_SCL_SECURE_NO_WARNINGS%0D%0A%20%20%20%20%20%20%20%20COMPILE_FLAGS%20%22/WX%22)%0D%0Aendif()%0D%0A%60%60%60%0D%0A%0D%0ACompiling%20with%20%60/WX%60%20will%20make%20MSVC%20treat%20warnings%20as%20errors.%0D%0A%0D%0AIf%20this%20change%20is%20deemed%20useful,%20I%20can%20pick%20it%20up.%0D%0A%0D%0A---%0D%0A%0D%0AAs%20an%20aside,%20the%20GCC/CLang%20%60COMPILE_FLAGS%60:%20%60-Wall%20-Wextra%20-Wpedantic%60%20also%20raise%20the%20warning%20level%20of%20the%20compiler.%0D%0A%0D%0AThis%20is%20also%20possible%20for%20MSVC.%20The%20default%20warning%20level%20for%20MSVC%20is%20%60/W3%60,%20this%20can%20be%20set%20to%20%60/W4%60%20or%20%60/Wall%60.%20I%20experimented%20with%20those%20levels%20and%20I%20do%20NOT%20propose%20setting%20those%20higher%20levels.%20E.g.%20with%20%60/WX%20/W4%60%20the%20following%20code:%20%0D%0A%0D%0A%60%60%60cpp%0D%0Achar%20capital_a%20=%20std::toupper('a');%0D%0A%60%60%60%0D%0A%0D%0AWill%20fail%20to%20compile%20because%20the%20compiler%20warns%20(as%20error)%20about%20a%20possible%20loss%20of%20precision%20when%20setting%20%60capital_a%60%20(a%20%60char%60)%20to%20the%20return%20value%20of%20%60toupper()%60%20(an%20%60int%60).&category=cpp ) 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.

heijp06 commented 1 week ago

@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 :slightly_smiling_face:, 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 commented 1 week ago

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.