fix: enable runtime usage of inverse function by using compile-time unit extraction #726
Conversation
| #if MP_UNITS_API_NATURAL_UNITS | ||
| if constexpr (!MP_UNITS_ASSOCIATED_UNIT<MP_UNITS_REMOVE_CONST(decltype(To))>) | ||
| return (representation_values<Rep>::one() * one).force_in(To * q.unit) / q; | ||
| return (representation_values<Rep>::one() * one).force_in(To * get_unit(R)) / q; |
There was a problem hiding this comment.
I don't think those change anything.
I just checked your unit tests with the mainline implementation of math.h on clang-16, clang-21, gcc-12, and gcc-15. All of the inverse unit tests provided below compile without issues, requiring no changes to math.h.
Could you please recheck?
There was a problem hiding this comment.
I checked again with Clang 16, 17, and 21. These new tests compile with Clang 16 but not 17 or 21 (I think because of this macro https://github.com/mpusz/mp-units/blob/master/src/core/include/mp-units/bits/hacks.h#L125-L129).
Attached are build errors from Clang 21 with these tests.
inverse-error.txt
I originally noticed this error in CI for use of inverse in my Audio Examples PR (#644)
There was a problem hiding this comment.
Thanks!
I still don't think that the fix is right, though. The alternative get_unit(R) is not intuitive for users, and if it is the only way to make it correct, then we probably made a mistake in the design phase.
q.unit is intended to be a core constant expression and should work as expected. If it doesn't work in some compilers, then it is probably a bug. I have just checked all the supported compilers using this code: https://godbolt.org/z/PTcxf3ehn. clang 17 & 18 complain about it indeed, but it seems to be fixed in 20+. Maybe we should extend the range of the MP_UNITS_CONSTEVAL hack up to 18 instead?
There was a problem hiding this comment.
You could also check if quantity<R, Rep>::unit works.
rothmichaels
force-pushed
the
fix/optimal-inverse-runtime
branch
2 times, most recently
from
b6f46a7 to
45754e7
Compare
|
Is this fix still needed? |
…nit extraction Replace runtime unit access (q.unit) with compile-time type extraction (get_unit(R)) in the inverse function implementation. This prevents the function from becoming an immediate function when consteval unit operators are used, allowing inverse to work with runtime variables. Key changes: - math.h: Use get_unit(R) instead of q.unit in both code paths - test: Add comprehensive runtime inverse tests This surgical fix preserves maximum compile-time optimization while enabling DSP applications that need runtime inverse calculations. Fixes compilation error with Clang: "call to immediate function 'inverse' is not a constant expression" 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
rothmichaels
force-pushed
the
fix/optimal-inverse-runtime
branch
from
45754e7 to
507b2db
Compare
Yes this is still needed. @mpusz do you think this looks good to merge? (I didn't mean to close the PR just now — I hit the wrong button) |
2 participants
Replace runtime unit access (q.unit) with compile-time type extraction (get_unit(R)) in the inverse function implementation. This prevents the function from becoming an immediate function when consteval unit operators are used, allowing inverse to work with runtime variables.
Key changes:
This surgical fix preserves maximum compile-time optimization while enabling DSP applications that need runtime inverse calculations.
Fixes compilation error with Clang:
"call to immediate function 'inverse' is not a constant expression"
🤖 Generated with Claude Code