nholthaus / units

a compile-time, header-only, dimensional analysis and unit conversion library built on c++14 with no dependencies.
http://nholthaus.github.io/units/
MIT License
934 stars 134 forks source link

Add a way to specify units when "downcasting" to numeric type #303

Open Delgan opened 1 year ago

Delgan commented 1 year ago

Hi!

I love your library (currently using v2.3) and there is one feature I would like to suggest. Given a units type, I can convert it to int using unit.to<int>(). What about adding a second optional argument to specify a possible conversion to the desired unit type?

const auto secs units::time::seconds_t(1.5);
const auto ms = secs.to<int, units::time::milliseconds_t>();

This may look a bit useless but in my opinion, this is a huge semantic improvement than can avoid bugs in the following scenario:

// my_struct.h

struct MyStruct {
    int identifier;
    units::time::milliseconds_t elapsedTime;
};
// do_someting.h

void call_third_library(const MyStruct& my_struct) {
    // Call function from third library which expect an `int`
    third_library.set_time_in_milliseconds(my_struct.elapsedTime.to<int>());
}

Everything looks fine and it works well. But now, imagine a different developer decides for some reason to change MyStruct:

// my_struct.h

struct MyStruct {
    int identifier;
    units::time::seconds_t elapsedTime;
}; 

He changed elapsedTime from units::time::milliseconds_t to units::time::seconds_t without modifying do_something.h. Everything compile fine but now, we have a bug, because third_library.set_time_in_milliseconds() is called with the value in seconds.

For this reason, I always combine to with static_cast to ensure I'm downcasting my value from the expected type. I would prefer to use my_struct.elapsedTime.to<int, units::time::milliseconds_t> (or something similar) for two main reasons:

For reference, here is the solution proposed by mp-units: Working with Legacy Interfaces.

Guillaume227 commented 1 year ago

I have seen that problem too! I use the following pattern to make the code more robust. It doesn't involve that much more typing or any static_casting, just a copy constructor which hopefully is compiled away when the two types are the same (I didn't check it actually is).

void call_third_library(const MyStruct& my_struct) {
    // Call function from third library which expect an `int`
    third_library.set_time_in_milliseconds(millisecond_t(my_struct.elapsedTime).to<int>());
}
Delgan commented 1 year ago

@Guillaume227 You're right, actually static_cast is not needed at all. Thanks for pointing that out, it's definitely less verbose!

I still think that it could be nice for the library to provide a built-in way to do the two casts in one function call as it encourages and documents what I see as best practice. But surely, current solution is already good.