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

/MP not added to CMAKE_CXX_FLAGS for Visual Studio #2030

Closed
kaikai2 opened this issue Oct 17, 2017 · 3 comments · Fixed by #2031
Closed

/MP not added to CMAKE_CXX_FLAGS for Visual Studio #2030

kaikai2 opened this issue Oct 17, 2017 · 3 comments · Fixed by #2031

Comments

@kaikai2
Copy link
Contributor

kaikai2 commented Oct 17, 2017

In commit 1355b98 CMAKE_CXX_FLAGS is compared against a default value, however, the default value starts with a space which is not what you have when you run cmake to generate project files for visual studio.

Your Environment

  • Operating System and version: Windows 10
  • Compiler: VS2013
  • PCL Version: 1.8.1, 1.8.0
  • CMake: 3.8.1

Expected Behavior

When CMAKE_CXX_FLAGS is not specified explicitly from cmake, the /MP option should be added for Visual Studio to enable parallel compile.

Remove the leading space in that commit, and the /MP option is added as expected.

Current Behavior

Possible Solution

- if("${CMAKE_CXX_FLAGS}" STREQUAL " /DWIN32 /D_WINDOWS /W3 /GR /EHsc") ## Check against default flags
+ if("${CMAKE_CXX_FLAGS}" STREQUAL "/DWIN32 /D_WINDOWS /W3 /GR /EHsc") ## Check against default flags

Code to Reproduce

Context

@taketwo
Copy link
Member

taketwo commented Oct 17, 2017

Looks like a bug. @UnaNancyOwen any opinion on this?

@UnaNancyOwen
Copy link
Member

Yes, I think so, too. This is bug.
(In addition, /EHsc is multiple defined. And, some extra space is included. It is better to remove them.)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 7d033aa54..fbb4e0424 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -146,8 +146,8 @@ endif()
 
 if(CMAKE_COMPILER_IS_MSVC)
   add_definitions("-DBOOST_ALL_NO_LIB -D_SCL_SECURE_NO_WARNINGS -D_CRT_SECURE_NO_WARNINGS -DNOMINMAX -DPCL_ONLY_CORE_POINT_TYPES /bigobj ${SSE_DEFINITIONS}")
-  if("${CMAKE_CXX_FLAGS}" STREQUAL " /DWIN32 /D_WINDOWS /W3 /GR /EHsc")	# Check against default flags
-    SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /bigobj /EHsc /fp:precise /wd4800 /wd4521 /wd4251 /wd4275 /wd4305 /wd4355 ${SSE_FLAGS}")
+  if("${CMAKE_CXX_FLAGS}" STREQUAL "/DWIN32 /D_WINDOWS /W3 /GR /EHsc")	# Check against default flags
+    SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /bigobj /fp:precise /wd4800 /wd4521 /wd4251 /wd4275 /wd4305 /wd4355 ${SSE_FLAGS}")
 
     # Add extra code generation/link optimizations
     if(CMAKE_MSVC_CODE_LINK_OPTIMIZATION)
@@ -157,12 +157,12 @@ if(CMAKE_COMPILER_IS_MSVC)
     endif(CMAKE_MSVC_CODE_LINK_OPTIMIZATION)
     # /MANIFEST:NO") # please, don't disable manifest generation, otherwise crash at start for vs2008
 
-    if( MSVC_VERSION GREATER 1500 AND ${CMAKE_VERSION} VERSION_GREATER "2.8.6")
+    if(MSVC_VERSION GREATER 1500 AND ${CMAKE_VERSION} VERSION_GREATER "2.8.6")
       include(ProcessorCount)
       ProcessorCount(N)
       if(NOT N EQUAL 0)
-        SET(CMAKE_C_FLAGS   "${CMAKE_C_FLAGS}   /MP${N} ")
-        SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /MP${N} ")
+        SET(CMAKE_C_FLAGS   "${CMAKE_C_FLAGS} /MP${N}")
+        SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /MP${N}")
       endif()
     endif()
   endif()

@kaikai2 Thank you for reporting a bug. It’s great! If it is possible, Please send a pull request.

@kaikai2
Copy link
Contributor Author

kaikai2 commented Oct 17, 2017

@UnaNancyOwen I've send a pull request with the changes you mentioned.

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.

3 participants