`percent_t` implicit conversion and construction are off by a factor of 100

[chiphogg]

percent_t is implicitly constructible from numeric types, and implicitly convertible to double. On construction, it interprets the value as a percentage. On implicit conversion, it turns it into a scalar. These values differ by a factor of 100.

Why do we care? For one, if bidirectional implicit conversions exist, it's very surprising to end users if we can pick up factors of 100.

Furthermore, this creates ambiguity ("is this value a percentage? or a raw scalar?") and harms callsite readability. Consider this test:

TEST(percent_t, ImplicitConstructionCallsiteReadability) {
    const auto scaled_value = scale_by(0.75, 4);
    EXPECT_EQ(scaled_value, 3);
}

It seems reasonable enough. And this API also seems reasonable:

double scale_by(units::concentration::percent_t factor, double value) { return factor * value; }

But when we combine them, we get 0.03 instead of 3.

I think the right solution is to remove the implicit constructors of percent_t. If somebody is using a percentage type, they should make this clear at the callsite. I believe other units types (e.g., meter_t) already work this way. I think percent_t should join them.

---

Please include the following information in your issue:

1. Which version of units you are using: v2.3.1
2. Which compiler exhibited the problem (including compiler version): gcc 7.5.0

[ts826848]

For what it's worth, the v3.x branch also displays this behavior.

I'm not sure removing implicit constructors would completely solve the problem either, since an explicit constructor still runs into the percentage-vs-scalar ambiguity you mentioned:

TEST(percent_t, ImplicitConstructionCallsiteReadability) {
    const auto scaled_value = scale_by(static_cast<percent_t>(0.75), 4); // 75% or 0.75%?
    EXPECT_EQ(scaled_value, 3);
}

Disabling constructors entirely and providing static factory methods (e.g., percent_t::from_percentage/percent_t::from_scalar) would be the best solution in terms of removing ambiguity, but that's a somewhat inelegant solution due to needing to write special-case code.

[chiphogg]

Good point. I wonder if part of the answer is that the static_cast format (which I hadn't previously considered) isn't idiomatic? I would expect users to write something more like this:

TEST(percent_t, ImplicitConstructionCallsiteReadability) {
    // const auto scaled_value = scale_by(percent_t{0.75}, 4); // Oops! Clearly 0.75%.
    const auto scaled_value = scale_by(percent_t{75}, 4);  // Clearly 75%.
    EXPECT_EQ(scaled_value, 3);
}

This looks pretty unambiguous to me. But I think you're right that if somebody uses the static_cast format, it makes their code hard to understand.