-
Notifications
You must be signed in to change notification settings - Fork 540
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
Clean up CMake configuration and address compiler warnings. #12
Conversation
target_compile_options(${TARGET} PRIVATE | ||
-std=c++11 -fno-exceptions -fno-rtti) | ||
# For good call stacks in profiles, keep the frame pointers. | ||
if(NOT SPIRV_PERF STREQUAL "") |
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 realize now this should have been a binary option instead of a string option. Let's fix it later.
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.
Good catch! Fixed.
endif() | ||
elseif(WIN32) | ||
set(SPIRV_WARNINGS -D_CRT_SECURE_NO_WARNINGS /wd4800) |
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.
Is it wise to turn use -D_CRT_SECURE_NO_WARNINGS through the entire project?
I recall the discussion around the use of sscanf text/HexFloat.cpp that we can inspect should always be safe.
If it's just for that one place, I'd want to leave it just in that one place.
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.
Andrew will have another patch to deal with it.
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.
Hm. I see this was just ported from a different spot in the file.
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.
Yeah, the reason we had to add it in the tests is because these were not being added to the tests at all, so we had to manually add. I am going to put up a PR tomorrow that hopefully cleans this up. Also adds /WX (similar to -Werror)
-1 just for the minor request to remove the unused constructor argument for Disassembler. |
@dneto0: Done. Will squash before submitting. |
+2 |
- Removed dead configuration in CMakeLists.txt. - Used target_compile_options() instead of CMAKE_{C|CXX}_FLAGS. - Turned on warnings on tests. - Fixed various warnings for comparing signed with unsigned values. - Removed dead code exposed by compiler warnings.
CMakeLists.txt
.target_compile_options()
instead ofCMAKE_{C|CXX}_FLAGS
.