godot icon indicating copy to clipboard operation
godot copied to clipboard

Core: Implement Math equivalents of `typedefs.h` macros

Open Repiteo opened this issue 1 year ago • 3 comments

An alternative to #91283, where the typedefs.h math macros are setup for deprecation. Now each macro has a comment warning of future deprecation, and recommends using their new Math equivalents instead. The only function that already had an equivalent, abs, has been consolidated into a single constexpr template; the other functions were simply relocated. Both abs and sign were slightly refactored from their macros, while min, max and clamp ~~had their return type changed from auto to std::common_return_t<T...>~~ only use a single template parameter. The only changes to files outside of these were changing any instances of Math::absf and Math::absd to Math::abs, as those functions no longer exist after consolidating abs.

Repiteo avatar Apr 29 '24 17:04 Repiteo

What is the rationale for using std::common_type_t ?

I think it would be better to use a single template parameter to generate simple overloads. This would reduce the number of overloads, would not increase the binary size more than necessary and would be less error prone to use.

ze2j avatar Apr 30 '24 14:04 ze2j

The reason is to avoid using auto, which is one of the few expressly disallowed^1 C++ features. Despite that, the macros are one of the few areas which used that keyword, so I wanted to avoid that when relocating. From what I've tested, auto resolves to std::common_type_t in that instance; so it was a matter of making it explicit.

I'm not totally against using only a single type for the parameters, as that would eliminate needing std::common_type_t and consequently the <type_traits> header. However, I'm not certain what the full ramifications of that would be for converting existing macros over to the new system. It's not a dealbreaker if it ends up being disruptive, as it's already gonna be inherently disruptive to convert existing macros to the new functions.

Repiteo avatar Apr 30 '24 15:04 Repiteo

Changed to a single template parameter!

After some testing, it would indeed be somewhat disruptive, but not nearly as egregiously as I expected. Every case can be rectified by either specifying the template type right away (MAX<float>(intvar, floatvar)) or just doing the conversion within the function itself (MAX((float)intvar, floatvar)). This actually has the indirect benefit of ensuring casts are done ahead of time, so we entirely avoid situations where c++ would attempt mismatched signed/unsigned comparisons.

Repiteo avatar Apr 30 '24 16:04 Repiteo