search

openmopac / mopac

Molecular Orbital PACkage
http://openmopac.net
GNU Lesser General Public License v3.0
115 stars 31 forks source link

Fix handling of OpenMP. #77

Closed susilehtola closed 2 years ago

susilehtola commented 2 years ago

Fixes #76 .

Status

codecov-commenter commented 2 years ago

Codecov Report

Merging #77 (1610c3d) into main (54f56b5) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #77   +/-   ##
=======================================
  Coverage   68.23%   68.23%           
=======================================
  Files         330      330           
  Lines       71696    71696           
=======================================
  Hits        48925    48925           
  Misses      22771    22771
Impacted Files Coverage Δ
src/input/wrtkey.F90 64.13% <ø> (ø)
src/run_mopac.F90 78.04% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 54f56b5...1610c3d. Read the comment docs.

godotalgorithm commented 2 years ago

I'm a little bit hesitant to accept this, as I removed the use of find_package(OpenMP) in #74 because find_package(BLAS) and find_package(OpenMP) were finding different OpenMP libraries, and I was inadvertently linking the two libraries, which was causing weird problems. However, you are only using the find_package system to set OpenMP compiler flags here and not additionally linking an OpenMP library, which might be the correct middle ground. For normal usage of OpenMP, you would only normally specify the compiler flag anyways. The additional linking of libiomp5 is a specific nuance of the Intel MKL library that I don't fully understand.

godotalgorithm commented 2 years ago

This is just some nitpicking. I'm trying to be consistent about using target-based CMake settings rather than global settings whenever possible. Also, the case of xxx_FOUND is supposed to match the name of the package or library that you are looking for. However, OPENMP_FOUND instead of OpenMP_FOUND is such a common typo that it was explicitly added to the find_package(OpenMP) script as a redundant status variable.