Compilation and compilation time

[bansan85]

Hi, I saw you want feedback about mp-units. My comments have all already been addressed extensively but I think this really prevents the possibility of using the library in real projects.

I follow the evolution of mp-units for many years. I find the idea really good. I wanted to integrate it in some projects. But I never succeed.

I perfectly understand the idea "The library tries to be in the next standard so I use latest features". But even now, I can't build it with all newest gcc / clang / msvc despite the fact they almost implement all C++20 features. The library should have temporary backward compatibility. Disclaimer: no, I didn't tried very hard to build mp-units but every time I tried I spend an tough time.

Another problem is the compilation time. It's just a library to handle units ! It shouldn't take more that millisecond to convert from m/s to km/hr. It takes about 25s (all library) / 10s (when just including the specific header). It's not possible for real project. Even in a big Qt project, it doesn't take that time to build a single cpp file. I know that template can be hard for compiler (I already faced to. I had to remove some recursive template to fix it) but we have to live with it. I read you hope that modules could help with that. I think / hope module can help to reduce compilation time (like pch) but it shouldn't be the solution.

I know these 2 facts will be really difficult to fixed. I found issues about it from the beginning of the project. And since I can't integrate it in my project, I can't give feedback about his usage.

But I'm still really excited by this project and I will keep an eye on it ❤️

[mpusz]

Hi @bansan85,

Thanks a lot for your feedback! ❤️

What you write is true. We hope those issues will improve soon, but for now, we focus on other aspects of the project, like the remaining missing key framework features (both design and implementation) or the C++ standardization effort, which has taken most of my time lately, so I do not have that much time to write code anymore 😢.

I perfectly understand the idea "The library tries to be in the next standard so I use latest features"

Yes, we will continue to try to benefit from the latest features in the language to deliver the best API for C++29 (hopefully 😉). However, with every change like that, we will always try to provide backward C++20 compatibility. We were successful so far and plan to continue that way. You can find more about this in our brand new documentation chapter that I pushed yesterday.

Regarding the compilation issues, the library compiles fine on GCC and clang for a long time now. The last remaining to join the party is MSVC. Unfortunately, this compiler is really buggy and does not compile lots of conforming C++20 code. Our library is not the only one here. For example stdexec repo with the reference Senders/Receivers implementation does not compile with MSVC as well. We have tried several times to improve the situation here. I contacted many MSVC developers, leaders, and managers to try to attract their attention and help them fix their bugs. We made great progress at CppCon 2023 doing pair programming with one of the MSVC developers, but it just surfaced more bugs when they fixed their code... Hopefully, C++20 implementation in MSVC will mature soon, but we can't do much more now.

Regarding compile times, I did not have time to do any optimizations yet. I am sure it might be greatly optimized, but I do not want to start doing that before we get a final version of the framework. The reason for this is that, for now, everything is implemented in a straightforward way that is easy to understand, maintain, and extend. Optimizations may change it, so I would like to ensure that the framework will not need major changes before I start optimizing its compile times. I am also not sure how you measure the compile-time performance. Please mind that in our examples, often the longest one to compile is actually the formatting library that "just handles text output" 😉. Using output streams provides much faster compile times. Also, including header files with only required system definitions or not using SI symbols (just units) will help will compilation times if modules are not used.

I would like to thank you again for your feedback. We really need to hear back from our users, and I hope we will be able to address your concerns soon.

[bansan85]

Compilation

the library compiles fine on GCC and clang for a long time now

Maybe I stopped too early. I didn't found the tutorial about building the library. Usually, I do CTRL+F "build" in the main page of github to find the build.md file or the build paragraph. So I missed the https://mpusz.github.io/mp-units/latest/getting_started/installation_and_usage/ . Maybe you should link installation instructions to this page.

By the way, I never used conan. Usually, I use only CMake. So I built/installed gls-lite and ran:

cd mp-units
mkdir build_gcc
cmake ../src -Dgsl-lite_DIR=/home/XXXXXX/prog/gsl-lite/install/lib/cmake/gsl-lite

And I had "Error. I need fmt". So I thought that mp-units used some features that gcc don't have and didn't want to install fmt library.

But after some investigations, I forced C++ version:

cmake ../src -Dgsl-lite_DIR=/home/XXXXXX/prog/gsl-lite/install/lib/cmake/gsl-lite -DCMAKE_CXX_STANDARD=20

And success.

Usually, I set CMAKE_CXX_STANDARD (or target_compile_features(... PUBLIC cxx_std_20)) in my CMakeLists.txt. I don't know (and didn't found) best practices about writing CMAKE_CXX_STANDARD in CMakeLists.txt.

So: compilation fix !! (for gcc without the need of fmt). Thanks

For MSVC, it build / install fine but I can't build the example in the README.md:

D:\tmp\mp-units\install\include\mp-units\reference.h(80,54): error C2275: 'Q': expected an expression instead of a type

Compilation time

I built the main example of the readme (with "#include") and it took 4s. It's long but can be acceptable for a not too big project.

By the way, maybe you should add in the main example "#include" ? I know they has been removed but module may be a bit difficult to use nowadays.

#ifdef MP_UNITS_MODULES
import mp_units;
#else
#include <mp-units/systems/isq/isq.h>
#include <mp-units/systems/si/unit_symbols.h>
#include <mp-units/systems/international/international.h>
#include <mp-units/format.h>
#include <mp-units/ostream.h>
#endif

I didn't tried module.

Unit consistency

I already asked the question years ago but I couldn't find it (edit: find it). What's important for me is to have a variable that handle unit consistency but support multiple unit.

The example should work (but it doesn't)

quantity v1 = 110 * km / h;
v1 = 10 * m / h; // Should be good but is actually bad
v1 = 10 * s / h; // Should be bad and is actually bad

IMHO, this feature is mandatory for a unit library.

Good luck for the development of this library. I know that C++ is a tough language and may be time consuming.

[mpusz]

I am happy that you made the library to compile.

Maybe you should link installation instructions to this page.

I always thought that the https://github.com/mpusz/mp-units?tab=readme-ov-file#documentation chapter in README is enough 😉

Usually, I set CMAKE_CXX_STANDARD (or target_compile_features(... PUBLIC cxx_std_20)) in my CMakeLists.txt. I don't know (and didn't found) best practices about writing CMAKE_CXX_STANDARD in CMakeLists.txt.

In general, everything that starts with CMAKE_ should not be set from within the CMake's files but externally from the command line or through CI scripts. Setting CMAKE_CXX_STANDARD from the script is even worse as it may affect the API/ABI of the library. This is why it should be set consistently for the entire project and its dependencies. That is why Conan does a great job here to ensure that the entire pipeline uses consistent flags.

By the way, maybe you should add in the main example "#include" ?

I really would not like README and the documentation to duplicate the contents. I actually thought that I should remove the examples from the README file at all. But I decided to keep them to provide a rough look and feel. In the proper docs you have examples with both modules and headers: https://mpusz.github.io/mp-units/latest/getting_started/look_and_feel/. I spend plenty of time writing the documentation for our users 😉

What's important for me is to have a variable that handle unit consistency but support multiple unit.

I do not know what you mean here 😢 The example you mentioned does not work because a conversion from m/h to km/h is truncating and that is not allowed on integral representation types. If you used a floating point value, it would work:

quantity v1 = 110. * km / h;
v1 = 10 * m / h; // Works
v1 = 10 * s / h; // Should be bad and is actually bad

[mpusz]

But after some investigations, I forced C++ version:

I am not sure if that is needed as target_compile_features is used in every target:

mp-units/src/cmake/AddMPUnitsModule.cmake

Line 65 in 832cba8

target_compile_features(${target_name} ${${projectPrefix}TARGET_SCOPE} cxx_std_20)

[mpusz]

For MSVC, it build / install fine but I can't build the example in the README.md:

Yes, that is the current state of C++20 support in MSVC 😢

[bansan85]

quantity v1 = 110. * km / h;
v1 = 10 * m / h; // Works

😃 So simple. Yes, float works better !

I can't decide whether I would rather convert 10 m/h to km/h (which is what the library does) or have the unit of v1 become m/h (which the library can't do).

And thanks for the documentation. I definitely should have spent more time on it... 😇