You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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?
constauto secs units::time::seconds_t(1.5);
constauto 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.hstructMyStruct {
int identifier;
units::time::milliseconds_t elapsedTime;
};
// do_someting.hvoidcall_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.hstructMyStruct {
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?".
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).
voidcall_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>());
}
@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.
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 toint
usingunit.to<int>()
.What about adding a second optional argument to specify a possible conversion to the desired unit type?
This may look a bit useless but in my opinion, this is a huge semantic improvement than can avoid bugs in the following scenario:
Everything looks fine and it works well. But now, imagine a different developer decides for some reason to change
MyStruct
:He changed
elapsedTime
fromunits::time::milliseconds_t
tounits::time::seconds_t
without modifyingdo_something.h
. Everything compile fine but now, we have a bug, becausethird_library.set_time_in_milliseconds()
is called with the value in seconds.For this reason, I always combine
to
withstatic_cast
to ensure I'm downcasting my value from the expected type. I would prefer to usemy_struct.elapsedTime.to<int, units::time::milliseconds_t>
(or something similar) for two main reasons:static_cast
or any other cast twice.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 usingstatic_cast
frommilliseconds_t
tomilliseconds_t
?".For reference, here is the solution proposed by
mp-units
: Working with Legacy Interfaces.The text was updated successfully, but these errors were encountered: