[Openmp-commits] [PATCH] D110354: Add option to build libomp library using Microsoft Visual C++ naming scheme.

[Joachim Protze via Phabricator via Openmp-commits]

protze.joachim added a comment.

Added several questions inline



================
Comment at: openmp/CMakeLists.txt:97
 
+option(OPENMP_BUILD_140 "Build dll with VC naming scheme." OFF)
 
----------------
What does the 140 mean? Is this supposed to be the llvm release number?
I suggest to use a more descriptive name for this Cmake variable and not to encode a version number into the variable name.


================
Comment at: openmp/runtime/CMakeLists.txt:359
+if (OPENMP_BUILD_140)
+  set(LIBOMP_LIB_NAME ${LIBOMP_LIB_NAME}140.${LIBOMP_ARCH})
+endif()
----------------
Similar as above, what is 140?
Probably combine LLVM_VERSION_MAJOR and LLVM_VERSION_MINOR?

Why not make destinction Debug/non-Debug here?


================
Comment at: openmp/runtime/src/CMakeLists.txt:162
+if(OPENMP_BUILD_140)
+  if(uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG")
+    set(LIBOMP_PDB_NAME ${LIBOMP_DEFAULT_LIB_NAME}140d.${LIBOMP_ARCH})
----------------
CMAKE_BUILD_TYPE is case sensitive. Why make it case-insensitive here?


================
Comment at: openmp/runtime/src/CMakeLists.txt:242-253
+  # On debug LIBOMP_IMP_LIB_FILE should be LIBOMP_IMP_LIB_FILE_DBG = ${LIBOMP_DEFAULT_LIB_NAME_DBG}${CMAKE_STATIC_LIBRARY_SUFFIX}
+  # 									${LIBOMP_DEFAULT_LIB_NAME}d${CMAKE_STATIC_LIBRARY_SUFFIX}
+  # and the ARCHIVE_OUTPUT_NAME of ompdbg should be ${LIBOMP_DEFAULT_LIB_NAME_DBG}${LIBOMP_LIBRARY_SUFFIX}${CMAKE_STATIC_LIBRARY_SUFFIX} 
+
+  if(OPENMP_BUILD_140)
+    if(uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG")
+      set(LIBOMP_IMP_LIB_FILE ${LIBOMP_DEFAULT_LIB_NAME}d${CMAKE_STATIC_LIBRARY_SUFFIX})
----------------
The IMP does not need the 140?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110354/new/

https://reviews.llvm.org/D110354