Modernize cmake and fix a GCC miscompilation bug

[jan-grimo]

Fix gcc miscompilation with empty string literal

• All entries in the symbols array are empty with gcc 9.3.0 if an empty string literal ("") starts the initializer list. Several tests fail in that case. Adding the zero-termination character fixes this

Modernize CMake, slightly restructure

• Adds a CMakeLists.txt to the external subfolder, creating the catch_main library that is consumed by the src/test subfolder. This allows replacement of CMAKE_SOURCE_DIR and CMAKE_BINARY_DIR by CMAKE_CURRENT_... for more portability.
• Use target_* prefixed CMake functions for non-global state mutation (increased portability)
• Reduce list of included directories to enable easier installation of all header sources
  • config.h is only needed by the tests, so generate that there and include it there only
  • Change libirc-cross-includes to libirc-prefixed includes and remove include/libirc from the includes list. Reduces the list of included directories without regression
  • Move external eigen plugin to within include/libirc: From external/eigen/plugins to include/libirc/plugins/eigen. Removes external folder from includes list, allowing cleaner install
• Modern target linking style: Including or linking with absolute paths instead of targets makes installed packages inflexible to relocation of their dependencies
• Add configuration and version files allowing use of find_package with installations of libirc

Things to discuss:

• CMake package naming: Maybe you prefer libirc to just irc?
• Maybe you want the Armadillo/Eigen dependency resolution not to be hardcoded into the installed configuration files. That's possible, too. Then we'd just have to add the dependency resolution to the tests and the config.cmake.in file separately.
• CMake package namespacing: Maybe you want the imported package to be namespaced? It's currently not. If someone either includes your project as a subdirectory or via find_package, the imported target is always named just irc.

[codecov[bot]]

Codecov Report

Merging #46 into master will increase coverage by 0.01%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #46      +/-   ##
==========================================
+ Coverage   97.97%   97.99%   +0.01%     
==========================================
  Files          20       21       +1     
  Lines        2376     2395      +19     
==========================================
+ Hits         2328     2347      +19     
  Misses         48       48              

Impacted Files	Coverage Δ
include/libirc/atom.h	100.00% <ø> (ø)
include/libirc/connectivity.h	96.62% <ø> (ø)
include/libirc/io.h	100.00% <ø> (ø)
include/libirc/irc.h	80.95% <ø> (ø)
include/libirc/mathtools.h	100.00% <ø> (ø)
include/libirc/molecule.h	100.00% <ø> (ø)
...ude/libirc/plugins/eigen/Matrix_initializer_list.h	100.00% <ø> (ø)
include/libirc/transformation.h	95.91% <ø> (ø)
include/libirc/wilson.h	96.27% <ø> (ø)
include/libirc/periodic_table.h	100.00% <100.00%> (ø)
... and 1 more

---

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 bfa79f9...401dfda. Read the comment docs.

[jan-grimo]

Is this behavior documented anywhere? I'll add more compilers to the CI in order to catch such issues early on.

I have to admit I fixed that bug by throwing stuff at the wall until it was resolved. I have a suspicion an empty string literal might be a bugged case for constexpr initialization of a char[]. The point is for the string to be binary embedded and a pointer to be placed in the array, right? But an empty string literal, what does the compiler do with that? Place a null pointer? I have no idea what's going on in GCC there. Clang seems to deal with it just fine.

Which advantages and disadvantages would the alternative bring?

Hard to say what constitutes advantages or disadvantages, it depends on what you think is the likeliest use-case for the library I suppose:

I would say an advantage of hardcoding dependencies selected at build/install time into config files would be that the chosen dependency is definitely there when a downstream project calls find_package, and no extra variable would have to be set in CMake prior to the call to find_package to select armadillo or eigen.

Downside to it is that "package" distributions would be different depending on the used dependency and unflexible in selection of dependencies.

Arguably, it doesn't make much of a difference. In either case, users will have to choose which dependency they want to use. Only if you plan on distributing as flexible a cmake package as possible, then not hardcoding the dependency would be valuable. If every user is going to build/install this manually, selecting their linalg dependency themselves, then I'd say it doesn't matter.

Adding a namespace is the best practice in this case?

I'm not qualified to judge that, I think. My personal opinion would be that given that this library exports only a single target, namespacing is arguably overkill, since namespacing is mainly supposed to prevent name conflicts.

[peterbygrave]

No problem with CMake namespacing. Thanks for reminder, I really need to get those additions pushed.

I learnt two new cmake things: if(TARGET ..) and get_filename_component(TEST_NAME ${TEST_SOURCE} NAME_WE), thanks 👍

[RMeli]

Thank you for the clarifications @jargonzombies

Arguably, it doesn't make much of a difference. In either case, users will have to choose which dependency they want to use. Only if you plan on distributing as flexible a cmake package as possible, then not hardcoding the dependency would be valuable.

I think at this stage there is not much difference as you mention and therefore there is no real need to change this. But in the future we might want to come back to this and remove the hard-coded dependencies.

My personal opinion would be that given that this library exports only a single target, namespacing is arguably overkill, since namespacing is mainly supposed to prevent name conflicts.

I tend to agree, it seems overkill. We might come back to this if we will ever change the name of the library. Thanks @peterbygrave for weighing-in.