feature request - allow disabling floating point implicit conversion

[ghorn]

au permits implicit floating point conversions by design, including conversions which reduce precision.

GCC and Clang have a -Wfloat-conversion flag (implied by -Wconversion) which prevents implicit conversions from double to float. Clang-tidy has the cppcoreguidelines-narrowing-conversions check (aliased by bugprone-narrowing-conversions) which also enforces this.

In a codebase with these warnings/checks enable, it was surprising to me when a au::Quantity<Unit, double> was implicitly cast to au::Quantity<Unit, float>. It led to a subtle bug.

I'm guessing that there is no desire to remove the implicit conversions feature. On the other hand, in a codebase with -Wfloat-conversion / cppcoreguidelines-narrowing-conversions, the au::Quantity implicit conversions are essentially a "leak". What are your thoughts on this situation? How could it be possible for a codebase to disallow these implicit conversions? What would you think of something like a -DAU_NO_IMPLICIT_FLOAT_CONVERSIONS flag?

[chiphogg]

What a great question! You're right that this behavior was by design. And your argument about -Wfloat-conversion is a very compelling reason to revisit that design.

Au 0.5.0 added some new truncation docs that discuss our philosophy in more detail. I think the flaw in the doc's argument is that it only really works when you go from integers to floating point. Going from one floating point type, to another with wider tolerance (lower precision), probably is morally equivalent to truncation.

As to a flag based approach: that's something we strongly want to avoid. We don't have any configurable preprocessor macros in the library, and we'd have a pretty high bar to change that, as it feels like a slippery slope. So, assuming we don't add a flag, what are our options?

1. Stick to the status quo.
2. Treat narrowing float-to-float conversions as "truncating" everywhere.
3. Find some way to "hang" a customizable default policy on either the U or R template parameter.
4. Add a new operation for "implicit conversion", as an alternative to the "static cast" operation

(1) is definitely a live possibility, but I would rather find a way to satisfy your request if at all feasible. (2) would incur a tremendous upgrade burden, and probably also an ongoing usability burden: it's very common to initialize a float from a literal without an f suffix, for example. (3) is a possibility I've thought of for a variety of reasons over time, as a possible evolution direction for the library. mp-units has a "quantity spec" instead of our "unit", and it's a great design! Probably what I would do if I were starting all over. But even if we did this, it wouldn't be a "project wide" setting; you'd have to pick specific "strict" versions of units, or wrapped rep types. So it probably entails too much manual work to be appealing to you.

I think (4), which I just thought of now, may be the most interesting idea, if we can get it to work. We've got this brand new mechanism in 0.5.0 where every unit conversion is a sequence of abstract operations: static casts, multiplications, and dividing by integers. The "static cast" operation was basically standing in for all type conversions, but it doesn't have to be like this. Maybe we could add an "implicit conversion" operation, if we could reliably tell the difference from the invoking context (which I think we could).

The ideal end state would be that for users with -Wconversion enabled, we would raise compiler warnings/errors in exactly the same situations for Quantity as for the underlying raw numeric types. And for users without this warning enabled, the compiler would continue to accept them without complaint.

I'll put it in the 0.6.0 milestone not because I expect to be able to deliver it, but so that I can explore our options more fully. Of course, if we do deliver it, that'll be great. 🙂

[geoffviola]

Good discussion 😄 There is some overlap with MISRA C++ 2023 guidelines.

Rule 7.0.6 Assignment between numeric types shall be appropriate ... ... the source size shall be smaller than the target size... ...

flt2 = 0.0;  // Non-compliant - different sizes and not an
             // integral constant expression
flt3 = 0.0f; // Compliant

[ghorn]

The ideal end state would be that for users with -Wconversion enabled, we would raise compiler warnings/errors in exactly the same situations for Quantity as for the underlying raw numeric types.

I agree, that would be ideal. I assume the approach would be to have the conversion happen implicitly as opposed to with static_cast, is that right? I'm unclear exactly how it this works, but third party libraries are often included with -isystem and #include <>, which masks compiler warnings in those headers. Would -isystem mask the warning in this case? Additionally, I've noticed that some conversion operations in template functions are treated differently and more permissively by static analysis, I'm not sure if this case is affected.

If this approach wouldn't work for these or other reasons, I don't know of another approach to get this behavior, do you?

Question - if we took the approach in (2), would it be forcing stricter -Wconversion behavior on users who are not using that warning flag?

(2) would incur a tremendous upgrade burden, and probably also an ongoing usability burden: it's very common to initialize a float from a literal without an f suffix, for example.

I agree, though anecdotally the big codebase I work on prevents this by linting.

As to a flag based approach: that's something we strongly want to avoid

I think it's wise to do as little as possible in the pre-processor, though I wonder if it's the least evil way to support "opt-in" behavior. The correct opt-in approach would be (3), right? That seems like a major piece of work, no?

If we wanted to hack in this behavior today, I wonder if there is a relatively simple patch we could apply. Does the floating truncation happen in a single place, for instance as a template specialization, where we could just patch it out? That would be similar to a flag-based approach, except completely unsupported. It would be useful for an audit of our codebase if not for general usage.