Skip to content

[Units] Require target unit to be specified when reading value

Davide Basso requested to merge units-force-target into main

Coming back again with yet another MR regarding Units 🙃 . This time the MR is just a little bit more complex technically speaking.

The issue

I noticed that Units are commonly used in configuration parameters. Lets make an example:

#include <units/Time.h>

namespace MyConfig
{
using namespace Boardcore::Units::Time;

constexpr auto DEFUSE_TIMEOUT = 1_s;
}

Assume somewhere else that config is used in a context where it is not required as a Unit:

using namespace MyConfig;

void main()
{
    startBomb(5_s);
    Thread::sleep(DEFUSE_TIMEOUT.value() * 1000);
    defuse();
}

Now, let's say in the future DEFUSE_TIMEOUT needs to be changed to 0.5s. One could think of doing the following:

constexpr auto DEFUSE_TIMEOUT = 500_ms;

However, now the thread would not sleep for 500ms, but for 500.000! At the moment you need to check everywhere that Unit is used to make sure the new ratios still match.

Current solutions

The current way to prevent this issue is to do the following:

Thread::sleep(Millisecond{DEFUSE_TIMEOUT}.value());

However, this is not required by design, and it is easily forgotten, especially if someone doesn't know about this issue.

Another alternative, taking advantage of the current template argument of value() is:

Thread::sleep(DEFUSE_TIMEOUT.value<std::ratio<1, 1000>>());

This is still not required by design, and it is also quite verbose.

Proposed solution

The value() function has been modified to accept a template argument of type Unit. Now, you just need to do the following:

Thread::sleep(DEFUSE_TIMEOUT.value<Millisecond>());

A value() implementation without any template argument has been kept, but it is marked as deprecated.

Merge request reports