Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Delgan opened this issue Dec 17, 2022 · 2 comments
Open

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

Delgan opened this issue Dec 17, 2022 · 2 comments

Comments

@Delgan
Copy link
Contributor

Delgan commented Dec 17, 2022

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:

  • it's much less verbose than using static_cast or any other cast twice.
  • it can be explained in the units docs why it's useful for and thus suggested as best practice in our code base. It can be confusing otherwise for the reader who may ask "why using static_cast from milliseconds_t to milliseconds_t?".

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

@Guillaume227
Copy link

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
Copy link
Contributor Author

Delgan commented Dec 18, 2022

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants