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: Add constexpr constructors/operators to math structs #98768

Merged
merged 1 commit into from
Mar 17, 2025

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Nov 2, 2024

In the same vein as #92059, the goal with this PR is to exclusively change operators that could be converted to constexpr as-is; anything that would necessitate other functions/calls were simply left untouched. Similarly, the actual structure of the code itself was left untouched where possible, excluding only the cases where an operator could be migrated to the header. Beyond that, there were two other adjustments in this PR:

  • Adding a [[nodiscard]] annotation to operators returning primative types Nah
  • Converting test case math variables to constexpr & checks to static_assert where possible

@Repiteo Repiteo added this to the 4.x milestone Nov 2, 2024
@Repiteo Repiteo requested review from a team as code owners November 2, 2024 18:49
@Repiteo Repiteo removed request for a team November 2, 2024 18:49
@adamscott
Copy link
Member

I'll do a test to see how it affects Web builds size. Could be interesting if it reduces size, less if it bloats the builds.

@adamscott
Copy link
Member

I'll do a test to see how it affects Web builds size. Could be interesting if it reduces size, less if it bloats the builds.

From my test, it doesn't really seem to affect build size at all. So I'm all for the PR.

@clayjohn
Copy link
Member

clayjohn commented Nov 5, 2024

I'm not sure about this. I think moving more towards constexpr could be helpful, but we should be adding it in response to an actual need of the project. We shouldn't be adding in new C++ features just because. A large reason why we stick to older C++ is because it can be read and understood by a much wider range of people.

Our best practices for contributors highlight that "the problem must come before the solution" and "the problem must exist in the first place" In other words, we shouldn't be anticipating problems and then trying to solve them.

I think this PR is well-intentioned and is doing something that might be helpful, but I don't think it is solving any problem and is trying to get ahead of something that might be a problem in the future.

Similarly with [[nodiscard]], by adding it indiscriminately, we are adding noise to the git repo, adding compile time to CI and local projects, and complicating the codebase for other contributors. But we aren't solving any problems. [[nodiscard]] is helpful in some cases (which is why we use it in some cases), but we should only be applying it when we need it. Its not something we should just add it everywhere just because.

@Repiteo
Copy link
Contributor Author

Repiteo commented Nov 5, 2024

I can concede the [[nodiscard]] additions might be superflous and probably are too noisy. I'm fine removing those

constexpr is a bit trickier. If it was like the original proposal, where I tried overhauling the style & syntax of several functions to accomodate it, then I'd be more inclined to agree; that's part of the reason I turned the original proposal into a draft. However, the more focused nature of this and the dependancy PR makes changes that can be impacted by them much more obvious to integrate. The file changes in the dependancy were a result of unused variables now being discovered, and the test changes here were a result of being able to evaluate test conditions at compile-time. I'd argue both of these, while niche, are benefits of constexpr that the repo is taking immediate use of

This might not be a direct response to a "problem" needing to be solved, but I wouldn't say that it's trying to be that either. This feels more in-line with codestyle/QOL PRs, where the benefits come from the repo itself having more functionality/readability. constexpr is mercifully compact compared to a lot of other modern C++ features, being LESS verbose than current implementations on occasion (static _FORCE_INLINE_constexpr), so it generally stands alone in terms of the kind of stuff I'd actively want to change to more modern C++ (second only to type_traits)

@Repiteo Repiteo force-pushed the core/constexpr-math-operators branch from 0b59e8c to a63ef23 Compare November 5, 2024 21:20
@adamscott
Copy link
Member

Our best practices for contributors highlight that "the problem must come before the solution" and "the problem must exist in the first place" In other words, we shouldn't be anticipating problems and then trying to solve them.

While I agree with the premise, I think we should be cautious applying this principle relative to code style changes. Especially if code changes simplify the code.

@Repiteo Repiteo force-pushed the core/constexpr-math-operators branch from a63ef23 to 6f90309 Compare November 9, 2024 21:40
@Repiteo Repiteo force-pushed the core/constexpr-math-operators branch from 6f90309 to 5b13725 Compare November 9, 2024 21:40
@Repiteo Repiteo force-pushed the core/constexpr-math-operators branch from 5b13725 to 05802ef Compare November 10, 2024 00:58
@Repiteo Repiteo force-pushed the core/constexpr-math-operators branch from 05802ef to aec85e6 Compare November 26, 2024 02:28
@Repiteo Repiteo force-pushed the core/constexpr-math-operators branch from aec85e6 to 3730321 Compare March 16, 2025 14:25
@Repiteo Repiteo changed the title Core: Add constexpr operators to math structs Core: Add constexpr constructors/operators to math structs Mar 16, 2025
@Repiteo
Copy link
Contributor Author

Repiteo commented Mar 16, 2025

Now that #92059 is closed, this functionally supersedes that PR; renamed the title/commit message to include constructors as well

@Repiteo Repiteo modified the milestones: 4.x, 4.5 Mar 16, 2025
@Repiteo Repiteo force-pushed the core/constexpr-math-operators branch 2 times, most recently from 7ed9246 to c66ea9f Compare March 17, 2025 16:21
• Begin integrating `constexpr` on math tests; use `static_assert` where appropriate
@Repiteo Repiteo force-pushed the core/constexpr-math-operators branch from c66ea9f to ea62170 Compare March 17, 2025 17:17
@Repiteo Repiteo merged commit 1f64260 into godotengine:master Mar 17, 2025
20 checks passed
@Repiteo Repiteo deleted the core/constexpr-math-operators branch March 17, 2025 21:10
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.

4 participants