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

Move CowData find, rfind and count to Span #103932

Merged
merged 1 commit into from
Mar 17, 2025

Conversation

Ivorforce
Copy link
Member

@Ivorforce Ivorforce commented Mar 10, 2025

The idea of this PR is to establish span_algorithms.h, where we can put algorithms that run on spans.

There are 3 that are currently defined in CowData, which could be better defined on Span. In this way, people owning a LocalVector (or perhaps even something else, like Array) could use e.g. spans::find to find data in the container without re-implementing the algorithm. This is especially important because the algorithms are optimized for speed, while new implementations probably won't be.
In addition, the new implementations are substantially faster (same improvement / reason as #102059).

If we agree to go ahead with this, we can move a lot more functions to the spans namespace, to start deduplicating Vector, LocalVector and String code.

Helps address godotengine/godot-proposals#5144

@Ivorforce Ivorforce requested a review from a team as a code owner March 10, 2025 20:54
@Ivorforce Ivorforce force-pushed the span-algorithms-header branch 3 times, most recently from 0891912 to 147fa5c Compare March 10, 2025 21:15
@AThousandShips AThousandShips added this to the 4.x milestone Mar 11, 2025
@Ivorforce Ivorforce force-pushed the span-algorithms-header branch 2 times, most recently from 7f2ec4e to e77561d Compare March 15, 2025 12:53
Copy link
Contributor

@Repiteo Repiteo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the core idea, but I'm not sure why we're opting for a separate header/namespace over methods within Span itself. Everything so far uses Span as the first argument, and based off the name I imagine that'll be the case for the overwhelming majority of functions, so separating them feels like unnecessary bloat. Even if certain functions don't need to be scoped under Span, the convention is generally to have accompanying classes/functions within the same file (Variant being the only real exception thanks to its insane size), so that code should all be in span.h regardless.

@Ivorforce
Copy link
Member Author

I really like the core idea, but I'm not sure why we're opting for a separate header/namespace over methods within Span itself. Everything so far uses Span as the first argument, and based off the name I imagine that'll be the case for the overwhelming majority of functions, so separating them feels like unnecessary bloat. Even if certain functions don't need to be scoped under Span, the convention is generally to have accompanying classes/functions within the same file (Variant being the only real exception thanks to its insane size), so that code should all be in span.h regardless.

Tbh., I also kind of dislike needing the Spans::func(span, ...) instead of just span.func(...). I'd put it all in extension headers if that was possible 😅

The reason I was even considering putting them in a separate header was compile times and decoupling. But perhaps that's premature optimization.

@Repiteo
Copy link
Contributor

Repiteo commented Mar 15, 2025

No clue how much compile times would be impacted—that's something I think needs more investigation overall—but decoupling is absolutely premature optimization; Span should be interlinked with functions designed explicitly for Span.

@Ivorforce Ivorforce force-pushed the span-algorithms-header branch 2 times, most recently from 05b7a40 to b23512f Compare March 15, 2025 17:37
@Ivorforce
Copy link
Member Author

Ivorforce commented Mar 15, 2025

Alright, I don't really disagree so I've just moved the functions to Span.
I've also removed the Size template parameter, which in hindsight isn't very useful.

@Ivorforce Ivorforce changed the title Move CowData find, rfind and count to new header span_algorithms.h Move CowData find, rfind and count to Span Mar 15, 2025
Copy link
Contributor

@Repiteo Repiteo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@Ivorforce Ivorforce force-pushed the span-algorithms-header branch from b37b0b8 to 49e8601 Compare March 16, 2025 02:31
@Repiteo Repiteo merged commit b1d13c5 into godotengine:master Mar 17, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Mar 17, 2025

Thanks!

@Ivorforce Ivorforce deleted the span-algorithms-header branch March 17, 2025 16:01
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