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 conversion and sign-conversion warnings. #71
Conversation
Only two of the fixes were for GSL itself, but since all conversion warnings have been fixed in the tests and these warnings have been enabled for the test suite, it should be easier to discover conversion warnings in GSL in the future.
Thanks @Sillamacka (ref. issue #59) |
@@ -59,7 +59,7 @@ elseif( "${CMAKE_CXX_COMPILER_ID}" MATCHES "GNU" OR | |||
target_compile_options( gsl-lite-cpp14.t PUBLIC -std=c++14 ) | |||
endif() | |||
|
|||
add_compile_options( -Wall -Wno-missing-braces -fno-elide-constructors ) | |||
set ( CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wno-missing-braces -fno-elide-constructors -Wconversion -Wsign-conversion -Wno-string-conversion" ) |
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.
What is the reason to change add_compile_options
to set
?
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'll be honest, I don't fully understand the difference, but when using add_compile_options
no warnings were thrown and make VERBOSE=1
did not show the warning settings on the command line.
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 looked it up, this is the issue with add_compile_options
:
Adds options to the compiler command line for targets in the current directory and below that are added after this command is invoked.
From this stack overflow answer
Isn't CMake wonderfully intuitive?
So it seems the CMakeLists.txt
needs to be a bit rearranged in general to fix this.
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.
Aha, thanks!
See also gsl-lite PR 71, gsl-lite/gsl-lite#71
See also gsl-lite PR 71, gsl-lite/gsl-lite#71
Only two of the fixes were for GSL itself, but since all conversion warnings
have been fixed in the tests and these warnings have been enabled for the test
suite, it should be easier to discover conversion warnings in GSL in the future.