Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core: Implement Math equivalents of typedefs.h macros #91324

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Apr 29, 2024

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.

@ze2j
Copy link
Contributor

ze2j commented Apr 30, 2024

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.

@Repiteo
Copy link
Contributor Author

Repiteo commented Apr 30, 2024

The reason is to avoid using auto, which is one of the few expressly disallowed1 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.

Footnotes

  1. https://docs.godotengine.org/en/stable/contributing/development/cpp_usage_guidelines.html#auto-keyword

@Repiteo Repiteo force-pushed the core/typedefs-macros-to-math_funcs branch from ea2a7aa to ded577b Compare April 30, 2024 16:14
@Repiteo
Copy link
Contributor Author

Repiteo commented Apr 30, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants