feat: add C++ modules support for Clang & Libc++

[JohelEGP]

Partially addresses #7. This should at least serve as a base for modules support in MSVC, according to https://www.reddit.com/r/cpp/comments/tgvgtp/c_modules_in_cmake_with_visual_studio/.

I tried making the downcasting facility work, to no avail. So this is based on #349.

This PR doesn't use the module beyond the static tests nor fetches the dependency (yet). Feel free to accept it as is or request them (and other things).

[gitpod-io[bot]]

[JohelEGP]

Limitations have been documented: https://github.com/JohelEGP/jegp.cmake_modules/#limitations.

[JohelEGP]

Apparently, not all is lost, as it might be possible to make the downcasting facility work with Clang modules: https://godbolt.org/z/jenGb5hM8. I just need to figure out why this works and attempt the same in mpusz/units.

[mpusz]

@JohelEGP, is this PR ready to merge (besides merge conflicts)? I thought that you still wanted to add to it.

[JohelEGP]

I have to address some of the comments above.

[JohelEGP]

While I work on further integration, this is how I have made use of this PR:

  ExternalProject_Add(
    mp-units
    GIT_REPOSITORY "${WAARUDO_mp-units_GIT_REPOSITORY}"
    GIT_TAG "${WAARUDO_mp-units_GIT_TAG}"
    CMAKE_ARGS ${mp_units_args} -DUNITS_AS_MODULES=TRUE -DUNITS_USE_LIBFMT=FALSE
    CMAKE_CACHE_ARGS ${cxx_args} ${jegp_args} ${waarudo_args}
    SOURCE_SUBDIR "src"
    CONFIGURE_HANDLED_BY_BUILD TRUE
    BUILD_COMMAND "${CMAKE_COMMAND}" --build <BINARY_DIR> --target mp-units-module
    INSTALL_COMMAND ""
    DEPENDS jegp.cmake_modules gsl-lite
    EXCLUDE_FROM_ALL TRUE)

This adds https://github.com/JohelEGP/jegp.cmake_modules/#jegpaddmodule to CMAKE_MODULE_PATH.

[JohelEGP]

https://github.com/Kitware/CMake/blob/v3.27.4/Help/dev/experimental.rst#c20-module-apis explains how to use CMake's experimental support for C++ modules. Change the v3.27.4 in the link for your CMake version.

~~Using Clang 18, I'm afraid there seems to be a bug:~~ It was because I was using Clang modules (for import std; and #include translation). Otherwise, it seems to work fine.

Error with Clang modules

/home/johel/Documents/C++/Forks/mpusz/units/test/unit_test/static/quantity_test.cpp:52:22: error: constraints not satisfied for class template 'quantity' [with R = mp_units::si::metre{{}}, Rep = short]
   52 | static_assert(sizeof(quantity<si::metre, short>) == sizeof(short));
      |                      ^~~~~~~~~~~~~~~~~~~~~~~~~~
/home/johel/Documents/C++/Forks/mpusz/units/src/core/include/mp-units/quantity.h:85:28: note: because 'RepresentationOf<short, get_quantity_spec(mp_units::si::metre{{}}).character>' evaluated to false
   85 | template<Reference auto R, RepresentationOf<get_quantity_spec(R).character> Rep = double>
      |                            ^
/home/johel/Documents/C++/Forks/mpusz/units/src/core/include/mp-units/bits/representation_concepts.h:82:28: note: because 'short' does not satisfy 'Representation'
   82 | concept RepresentationOf = Representation<T> && ((Ch == quantity_character::scalar && is_scalar<T>) ||
      |                            ^
/home/johel/Documents/C++/Forks/mpusz/units/src/core/include/mp-units/bits/representation_concepts.h:79:93: note: because 'short' does not satisfy 'Scalable'
   79 | concept Representation = (is_scalar<T> || is_vector<T> || is_tensor<T>)&&std::regular<T> && detail::Scalable<T>;
      |                                                                                             ^
/home/johel/Documents/C++/Forks/mpusz/units/src/core/include/mp-units/bits/representation_concepts.h:70:3: note: because 'short' does not satisfy 'CastableNumber'
   70 |   CastableNumber<T> ||
      |   ^
/home/johel/Documents/C++/Forks/mpusz/units/src/core/include/mp-units/bits/representation_concepts.h:65:62: note: because 'std::common_type_t<short, std::intmax_t>' (aka 'long') does not satisfy 'ScalableNumber'
   65 | concept CastableNumber = CommonTypeWith<T, std::intmax_t> && ScalableNumber<std::common_type_t<T, std::intmax_t>>;
      |                                                              ^
/home/johel/Documents/C++/Forks/mpusz/units/src/core/include/mp-units/bits/representation_concepts.h:62:3: note: because 'std::regular_invocable<std::multiplies<>, long, long>' evaluated to false
   62 |   std::regular_invocable<std::multiplies<>, T, U> && std::regular_invocable<std::divides<>, T, U>;
      |   ^
/home/johel/root/clang-main/bin/../include/c++/v1/__concepts/invocable.h:34:29: note: because 'invocable<std::multiplies<void>, long, long>' evaluated to false
   34 | concept regular_invocable = invocable<_Fn, _Args...>;
      |                             ^
/home/johel/root/clang-main/bin/../include/c++/v1/__concepts/invocable.h:28:3: note: because 'std::invoke(std::forward<_Fn>(__fn), std::forward<_Args>(__args)...)' would be invalid: no matching function for call to 'invoke'
   28 |   _VSTD::invoke(_VSTD::forward<_Fn>(__fn), _VSTD::forward<_Args>(__args)...); // not required to be equality preserving
      |   ^
/home/johel/root/clang-main/bin/../include/c++/v1/__config:805:17: note: expanded from macro '_VSTD'
  805 | #  define _VSTD std
      |                 ^
/home/johel/Documents/C++/Forks/mpusz/units/src/core/include/mp-units/bits/representation_concepts.h:71:24: note: and 'typename T::value_type' would be invalid: type 'short' cannot be used prior to '::' because it has no members
   71 |   (requires { typename T::value_type; } && CastableNumber<typename T::value_type> &&
      |                        ^
/home/johel/Documents/C++/Forks/mpusz/units/src/core/include/mp-units/bits/representation_concepts.h:73:24: note: and 'typename T::element_type' would be invalid: type 'short' cannot be used prior to '::' because it has no members
   73 |   (requires { typename T::element_type; } && CastableNumber<std::remove_reference_t<typename T::element_type>> &&
      |                        ^

[JohelEGP]

This is how I tested this (using CPM instead of Conan). In the CMake buildsystem:

CPMAddPackage("gh:gsl-lite/gsl-lite#4b5e9ab7474841fc2d7efc2e0064ef81785535d1")
CPMAddPackage("gh:fmtlib/fmt#e8259c5298513e8cdbff05ce01c46c684fe758d8")
CPMAddPackage("gh:catchorg/Catch2#3a5cde55b7a27a6d94ce994a8362b2425211bd5e")
execute_process(COMMAND "${CMAKE_COMMAND}" -S "${CPM_Catch2_SOURCE}" -B Catch2
                WORKING_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}" COMMAND_ERROR_IS_FATAL ANY)
set(Catch2_DIR "${CMAKE_CURRENT_BINARY_DIR}/Catch2")
list(APPEND CMAKE_MODULE_PATH "${CPM_Catch2_SOURCE}/extras")
CPMAddPackage(
  NAME "mp-units"
  VERSION "modules"
  GITHUB_REPOSITORY "JohelEGP/mp-units"
  OPTIONS "MP_UNITS_USE_LIBFMT TRUE"
  SOURCE_SUBDIR "src")
add_subdirectory("${CPM_mp-units_SOURCE}/test" "mp-units-test")
add_subdirectory("${CPM_mp-units_SOURCE}/example" "mp-units-example")

[mpusz]

This is how I tested this (using CPM instead of Conan).

We need to be able to trigger this from Conan.

[mpusz]

Hi @JohelEGP, it would be great to merge this PR but there are still some unresolved issues we discussed above.

[JohelEGP]

All that's left is the integration with Conan. I'll leave that to you.

I suppose you're going to add an option to Conan, which translates to a CMake option. You should use that in place of CMAKE_EXPERIMENTAL_CXX_MODULE_CMAKE_API.

https://github.com/fmtlib/fmt/releases/tag/10.1.1 is needed for the fixes for C++ modules. Of https://github.com/mpusz/mp-units/issues/7#issuecomment-1073138373, https://github.com/llvm/llvm-project/issues/54574 is still open, so warnings abound.

[JohelEGP]

Unsurprisingly, GCC 13 and GCC 14 ICE.

[mpusz]

The CI needs fixing as well. We should probably add modules tests to CI as well to check if there are no issues in the PR. Which compilers should the module-build support as of now? Only clang-16+?

When you are done with all the required changes and the CI will be fixed I will contribute to this PR with Conan-related changes to enable CI builds with modules.

[JohelEGP]

Looks like Clang 16 can't be used to test the modules.

[mpusz]

Looks like Clang 16 can't be used to test the modules.

I got the info from the Conan team that Conan 2.0.12 should appear today and will have fixes to enable clang-17 configuration.

[mpusz]

clang-17 support added with https://github.com/mpusz/mp-units/commit/92b7069c35bba592743c1132c4c2eccfa9c59c6c.

[mpusz]

WOW!!! This is so awesome! 🎉

[mpusz]

Please let me know when you will think it is ready to merge (i.e. in case you want to provide those toolchain files for CMake).

[jcar87]

Hi @mpusz - I've had a chance to test things out on this branch as I prepare to continue experiments with C++ module packaging.

A few comments:

• The MP_UNITS_BUILD_MODULES option is declared in the top-level CMakeLists.txt but is never evaluated - module functionality seems to be conditional on either of the following, which have some issues.

  • CMAKE_EXPERIMENTAL_CXX_MODULE_CMAKE_API being defined. Note that this is no longer needed in CMake 3.28 - module scanning functionality is enabled automatically when C++20 mode is active (as far as I'm aware).
  • or if(TARGET mp-units::modules) - this never entered the "true" branch for me when building this project with CMake 3.28 nightlies. Unsure why - this is ver dependent on CMake configuration order, but I can see the src directory is included before the others.

• My advice for the above would be to align everything around the MP_UNITS_BUILD_MODULES condition.

• Defining cmake_minimum_required() to an old version of CMake (e.g 3.4 or 3.5) - this was disabling the dependency scanning to that part of the tree - and I was getting a lot of import errors because CMake never ran clang-scan-deps for that file. Removing these lines did the trick. I'd say they're only needed at the top-level project, and any subdirectory that you expect to be "standalone" (that is, the root of a tree). Otherwise they're redundant at best, and dangerous at worse (since the call to cmake_minimum_required() changes the policy settings).

• having (lots) of compiler errors with Clang 17 and GNU's libstdc++ (clang on Linux uses the GNU standard library by default) - these could be due to issues specific to GNU's library, but worth cehcking

• ~using Clang 17 and libc++ works without compiler errors, but I get linker errors instead - I suspect that this may have to do things from #includes (standard library, GSL and fmt) being attached to the module. For everything that you do not want to be attached to the modules, you'll have to do an #include in the module preamble (after module; and before export module xxxx;. You can see an example here: https://github.com/fmtlib/fmt/blob/master/src/fmt.cc#L3-L36 - this will also apply to other things that are #included , such as fmt (and maybe gsl?)~ Update: set this up wrong, clang 17 and libc++ compiles, links, and test pass :D

[jcar87]

As for the cmake_minimum_required() - indeed this will be a problem with CMake 3.28 if the different subdirectories have a call to that function for a version earlier than 3.28, as described here:

• https://cmake.org/cmake/help/git-master/manual/cmake-cxxmodules.7.html
• https://cmake.org/cmake/help/git-master/policy/CMP0155.html#policy:CMP0155

There are a few alternatives:

• At the top level, evaluate MP_UNITS_BUILD_MODULES early on, and if true, set(CMAKE_CXX_SCAN_FOR_MODULES ON) - this will enable dependency scanning for all targets, regardless

• Alternatively (and probably better): ensure the policy default is new, even for files that set cmake_minimum_required() for older versions: set(CMAKE_POLICY_DEFAULT_CMP0155 NEW) - I think this ensures dependency scanning by default for all targets where C++20 or higher is used

• Removing calls to cmake_minimum_required() from files that don't need it (and inherit it from the parent) is probably the best approach. For the files that do need it, may be safer to include 3.28, e.g. cmake_minimum_required(3.5...3.28)

[mpusz]

I am still looking for the best modules structure for the project. There are a few alternatives:

The simplest one

flowchart TD
    mp_units --- systems --- core
    mp_units --- ostream --- core
    mp_units --- fmt --- core

If someone does not want any I/O (i.e. embedded), the following is needed:

import mp_units.systems;

Separate I/O options

flowchart TD
    mp_units --- systems --- core
    mp_units_ostream --- mp_units
    mp_units_fmt --- mp_units

In many cases, we use both I/O options (fmt and streams). In such a case, we need:

import mp_units_ostream;
import mp_units_fmt;

Unified I/O

flowchart TD
    mp_units --- systems --- core
    mp_units_ostream --- mp_units
    mp_units_fmt --- mp_units
    mp_units_io --- mp_units_ostream
    mp_units_io --- mp_units_fmt

Now we just need

import mp_units_io;

[mpusz]

I personally like the first one the most. It is the simplest one and allows selective inclusion of all the options.

[mpusz]

We could consider breaking the project into even smaller pieces, but I am unsure if it makes sense to provide separate modules for each system. It could lower the module compile times, though. However, I feel that in V2 systems of units are really cheap to compile and most of the effort will be in compiling the ISQ. Splitting the ISQ into submodules probably makes no sense?

[JohelEGP]

I'm going to limit the scope to C++ and CMake sources, as originally done.

[mpusz]

@jcar87, could you please help @JohelEGP with his issues with Conan support for modules? We would like to merge this to the mainline ASAP.

[JohelEGP]

I'd prefer to limit this to PR CMake. It was working before. But then I had to touch the Conan files, which I don't really understand.

Since the CMake buildsystems already work standalone, the Conan support should be purely additive.

[JohelEGP]

With regards to the first option of https://github.com/mpusz/mp-units/pull/350#issuecomment-1763842790, the module mp_units.core would only include the headers compare.h and random.h from the utility sources. The utility sources chrono.h and math.h require SI, so they would be part of mp_units.systems.

[JohelEGP]

I suppose all that's missing now is documentation on how to use it.

You need to follow https://cmake.org/cmake/help/latest/manual/cmake-cxxmodules.7.html. For the CMake buildsystems of the examples and tests in this repository, which don't require a minimum CMake version of 3.28, that means configuring with -DCMAKE_CXX_SCAN_FOR_MODULES="TRUE".

Right now, if the CMake version is at least 3.28, the modules targets are unconditionally added.

[mpusz]

With regards to the first option of #350 (comment), the module mp_units.core would only include the headers compare.h and random.h from the utility sources. The utility sources chrono.h and math.h require SI, so they would be part of mp_units.systems.

I think that math.h should also end up in core. We should probably split it into generic math and trigonometric functions. The latter should be attached to SI and angular systems.

I agree that chrono.h should probably be provided with the SI system or on top of it (if we ever decide to provide finer-grained modules).

[mpusz]

Please see:

• c9d1f83ca493931909392cf28334e11dfd251ff9
• 7f0e7f8ca86c5860d6c3e5c62eaddcd43092a78a