Skip to content

Remove Array include from dictionary.h and ustring.h.#111244

Merged
Repiteo merged 1 commit into
godotengine:masterfrom
Ivorforce:dont-include-array
Oct 6, 2025
Merged

Remove Array include from dictionary.h and ustring.h.#111244
Repiteo merged 1 commit into
godotengine:masterfrom
Ivorforce:dont-include-array

Conversation

@Ivorforce

Copy link
Copy Markdown
Member

Decreases coupling within core by removing array.h include from dictionary.h and ustring.h.
Most modules will include it anyway, since many are working with variant.h, which includes pretty much everything.

Still, this should improve compile time for some modules, and pave the way for the eventual variant.h includes refactor.

@Repiteo Repiteo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good!

@Repiteo Repiteo modified the milestones: 4.x, 4.6 Oct 5, 2025

@Repiteo Repiteo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

While testing a merge batch, other changes revealed further necessary forward-declares. The first is that we need to forward-declare Array in core/extension/gdextension_special_compat_hashes.h, while the second is shown below (alongside respecting the original file's choice to separate class/struct fds with a newline)

Comment thread core/variant/dictionary.h Outdated
@Ivorforce Ivorforce requested a review from a team as a code owner October 6, 2025 14:31
@Ivorforce

Copy link
Copy Markdown
Member Author

While testing a merge batch, other changes revealed further necessary forward-declares. The first is that we need to forward-declare Array in core/extension/gdextension_special_compat_hashes.h, while the second is shown below (alongside respecting the original file's choice to separate class/struct fds with a newline)

I had a feeling some of these include changes would clash 😅
Thanks for the pointers, I've pushed a version that builds again!

@Repiteo Repiteo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I had a feeling some of these include changes would clash 😅

Yeaaah, that's another inconvenience of this process. Piecemealed changes are great for atomic changelogs, but they won't always play nice if merged in batches. Attempting to not merge in batches can lead to this process taking too long, and risks stagnating several PRs that are completely acceptable in isolation.

I suppose there's always the option of a local validation step and making relevant changes there before pushing, but I can't guarantee that'll pass all CI as well. If anything, this could be the usecase for the native merge queue feature, albeit one that'd still need a healthy amount of pre-planning

In any case, assuming CI passes, this is once again ready to roll!

@Repiteo Repiteo merged commit f1f9f54 into godotengine:master Oct 6, 2025
20 checks passed
@Repiteo

Repiteo commented Oct 6, 2025

Copy link
Copy Markdown
Contributor

Thanks!

@Ivorforce Ivorforce deleted the dont-include-array branch October 6, 2025 19:56
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.

2 participants