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

Squelch Visual Studio's obnoxious warnings about sprintf being unsafe. #698

Merged
merged 2 commits into from Dec 14, 2017

Conversation

Projects
None yet
3 participants
@cffk
Contributor

cffk commented Dec 7, 2017

Also set policy CMP0054 to stop a cmake warning.
This fixes #695

Squelch Visual Studio's obnoxious warnings about sprintf being unsafe.
Also set policy CMP0054 to stop a cmake warning.
@cffk

This comment has been minimized.

Show comment
Hide comment
@cffk

cffk Dec 7, 2017

Contributor

Whoops, this is to fix #696 (not #695).

Contributor

cffk commented Dec 7, 2017

Whoops, this is to fix #696 (not #695).

@pstieber

This comment has been minimized.

Show comment
Hide comment
@pstieber

pstieber Dec 7, 2017

@cffk Wouldn't the addition of /wd4996 on line 20 of CMakeLists.txt be a simpler solution that wouldn't pollute the code base with Microsoft specific pragma lines?

The following works for me:

if("${CMAKE_C_COMPILER_ID}" STREQUAL "MSVC")
  set(CMAKE_C_FLAGS "/WX /wd4996 ${CMAKE_C_FLAGS}")
elseif("${CMAKE_C_COMPILER_ID}" STREQUAL "GNU")

pstieber commented Dec 7, 2017

@cffk Wouldn't the addition of /wd4996 on line 20 of CMakeLists.txt be a simpler solution that wouldn't pollute the code base with Microsoft specific pragma lines?

The following works for me:

if("${CMAKE_C_COMPILER_ID}" STREQUAL "MSVC")
  set(CMAKE_C_FLAGS "/WX /wd4996 ${CMAKE_C_FLAGS}")
elseif("${CMAKE_C_COMPILER_ID}" STREQUAL "GNU")
@cffk

This comment has been minimized.

Show comment
Hide comment
@cffk

cffk Dec 7, 2017

Contributor

I can go either way on this... The benefits of my approach:

  • targets just the files in question;
  • handles the case where the medicine needs to be applied to installed header files.
Contributor

cffk commented Dec 7, 2017

I can go either way on this... The benefits of my approach:

  • targets just the files in question;
  • handles the case where the medicine needs to be applied to installed header files.
@kbevers

This comment has been minimized.

Show comment
Hide comment
@kbevers

kbevers Dec 14, 2017

Member

After thinking about this for a while, I lean towards the "simple" solution of adding /wd4996 to the CMake flags. I prefer to have the code base as neutral as possible. Keeping the platform specifics (mostly) to header files and build systems achieve that goal. It makes things a little easier for for one-time contributors as well as allow us to govern the rules for warning- and error-handling centrally. Or at least as centrally as possible when maintaining three orthogonal build systems...

Member

kbevers commented Dec 14, 2017

After thinking about this for a while, I lean towards the "simple" solution of adding /wd4996 to the CMake flags. I prefer to have the code base as neutral as possible. Keeping the platform specifics (mostly) to header files and build systems achieve that goal. It makes things a little easier for for one-time contributors as well as allow us to govern the rules for warning- and error-handling centrally. Or at least as centrally as possible when maintaining three orthogonal build systems...

@cffk cffk merged commit 3ccc6c3 into OSGeo:master Dec 14, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.7%) to 74.048%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment