GeomScale / volesti

Practical volume computation and sampling in high dimensions
GNU Lesser General Public License v3.0
144 stars 113 forks source link

macos/ARM/clang compile errors #295

Open congyu711 opened 6 months ago

congyu711 commented 6 months ago

Describe the bug I find some bugs when compiling with /test/CMakeLists.txt on ARM based macos with clang.

I make several workarounds and now it compiles successfull. I don't know if it is necessary for a pull request and whether my workaround breaks the build on other platforms.

  1. in external/cmake-files/Eigen.cmake, suffixs should be added to find_path find_path(EIGEN_DIR NAMES signature_of_eigen3_matrix_library PATHS ${EIGEN_CMAKE_DIR}/../_deps/eigen-src PATH_SUFFIXES eigen3 eigen) , since eigen3 is contained in a subfolder and find_path doesn't find recursively.
  2. in test/CMakeLists.txt, add_definitions(${CMAKE_CXX_FLAGS} "-std=c++17") should be replaced with set(CMAKE_CXX_STANDARD 17). CMAKE_CXX_FLAGS is passed to all compile commands including some external projects in C. Passing c++ standard flags when compiling C files causes warnings in gcc but errors in clang.
  3. in external/cmake-files/LPSolve.cmake. find_path searches for lpsolve.h. However if LPSolve is installed with homebrew, /opt/homebrew/include/ contains lpsolve.h but not ${LP_SOLVE_DIR}/bfp
image

so if find_path finds the lpsolve.h in /opt/homebrew/include, the following include_directories and add_library commands in LPSolve.cmake won't work.

  1. start=std::chrono::system_clock::now() does not compile in clang if start's type is std::chrono::time_point<std::chrono::high_resolution_clock>. high_resolution_clock and system_clock have different precision. Related files:

    • include/ode_solvers/implicit_midpoint.hpp
    • include/preprocess/crhmc/crhmc_problem.h
    • test/benchmarks_crhmc.cpp
    • include/random_walks/crhmc/crhmc_walk.hpp
  2. in file external/PackedCSparse/FloatArray.h. #include <immintrin.h> causes compile errors. I think immintrin.h does not exists ARM machines. Removing this include seems to be fine.

vfisikop commented 6 months ago

Thanks, please open a PR with your changes. If the CI tests pass on your PR then it should be OK to merge it.