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

Remove unused headers in core #100564

Merged
merged 1 commit into from
Dec 20, 2024
Merged

Remove unused headers in core #100564

merged 1 commit into from
Dec 20, 2024

Conversation

YYF233333
Copy link
Contributor

Trying to apply include-what-you-use to godot codebase, but turns out it is not smart enough for such complex dependency graph, so I pick some safe changes and applied manually.

Hopefully this will reduce build time a bit. (and also warnings in your IDE) 😄

@YYF233333 YYF233333 requested review from a team as code owners December 18, 2024 16:04
@akien-mga
Copy link
Member

Seems like https://github.com/godotengine/godot/actions/runs/12396780139/job/34605644487?pr=100564 requires script_language.h in main.cpp.

@akien-mga akien-mga modified the milestones: 4.x, 4.4 Dec 18, 2024
@Repiteo Repiteo merged commit bf9ef5f into godotengine:master Dec 20, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 20, 2024

Thanks!

@@ -42,8 +42,6 @@
#include "core/templates/safe_refcount.h"
#include "core/typedefs.h"

#include <new>
Copy link
Member

@akien-mga akien-mga Jan 1, 2025

Choose a reason for hiding this comment

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

FYI, this was actually needed and caused a crash with GCC 12: #100768 (fixed in #101004).
This was a honest mistake but shows we need to be somewhat cautious when reviewing IWYU suggestions.

I'm still unsure why IWYU would consider it unused when it's used below, but I suspect it's a matter of #if conditions not matching. On some platforms (MSVC) this <new> would indeed be unused, while it's used for GCC (and presumably Clang).

So we need to be careful there too that some "global" includes might actually be needed for a code branch that's not compiled on the current platform used with IWYU. Normally CI should catch it and fail compiling, but in this case it didn't (I don't know why it compiles but then crashes).

@@ -31,8 +31,6 @@
#ifndef THREAD_SAFE_H
#define THREAD_SAFE_H

#include "core/os/mutex.h"
Copy link
Member

Choose a reason for hiding this comment

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

This was also a wrong suggestion from IWYU, as it's used just below in macros. Apparently it doesn't support macros, so we need to be cautious there too when reviewing changes.

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