Skip to content

Un-support force_trivial parameter for LocalVector. Instead, users should use resize_uninitialized. #106876

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

Merged
merged 1 commit into from
May 28, 2025

Conversation

Ivorforce
Copy link
Member

@Ivorforce Ivorforce commented May 27, 2025

The parameter force_trivial was always just a way to resize the LocalVector without calling default initializers (to save time).
Now, this is no longer required - users can simply call resize_uninitialized! This makes it explicit where a resize is happening without filling values.

This PR should not change behavior, just simplify matters and improve code style.

Notes

  • I've checked changed_items and _cull_hits, they never called resize so the force_trivial parameter was useless for both.
  • force_trivial behavior handling is bubbled up to PooledList. It is somewhat questionable whether it is appropriate here, because it is only used to create one object at a time. Still, this is outside the scope of this PR to change.
  • _active_map and freelist use a trivial type so they don't initialize on resize (regardless of force_trivial). I took the opportunity to make this explicit using resize_uninitialized, as we'll make that a goal soon anyway.

@Ivorforce Ivorforce added this to the 4.x milestone May 27, 2025
@Ivorforce Ivorforce requested a review from lawnjelly May 27, 2025 16:34
@Ivorforce Ivorforce requested a review from a team as a code owner May 27, 2025 16:34
…s are reformatted to use `resize_uninitialized` to make it explicit that the resize does not initialize missing elements.
@Ivorforce Ivorforce force-pushed the localvector-no-force-trivial branch from ce1283f to 3741553 Compare May 27, 2025 16:43
template <typename T, typename U = uint32_t, bool force_trivial = false, bool tight = false>
class LocalVector {
static_assert(!force_trivial, "force_trivial is no longer supported. Use resize_uninitialized instead.");

Copy link
Member

Choose a reason for hiding this comment

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

Oof yeah, not being able to remove force_trivial easily is a pain for backward compatibility with third party stuff.

Copy link
Member Author

@Ivorforce Ivorforce May 27, 2025

Choose a reason for hiding this comment

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

Since we may want to remove the tight parameter too in favour of something like reserve_exact, we may have the opportunity to remove both at once :)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I've never thought the tight parameter was a good idea, and suggested reserving to exact size myself around that time it was added.

Copy link
Member

@lawnjelly lawnjelly left a comment

Choose a reason for hiding this comment

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

Assuming passes CI...

@Ivorforce Ivorforce modified the milestones: 4.x, 4.5 May 27, 2025
@Repiteo Repiteo merged commit 2cde929 into godotengine:master May 28, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented May 28, 2025

Thanks!

@Ivorforce Ivorforce deleted the localvector-no-force-trivial branch May 28, 2025 14:50
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