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 servers #100634

Merged
merged 1 commit into from
Dec 20, 2024
Merged

Conversation

YYF233333
Copy link
Contributor

Followup #100564

Cleanup includings in servers/, remove unused headers, replace headers used for transitive include, move headers down in the include chain if possible.

@akien-mga
Copy link
Member

Seems like modules/camera/camera_feed_linux.cpp needs to include RenderingServer, and for some reason there's a new warning in an iOS file triggered by these changes (likely a pre-existing warning that just happened to be raised now because some header changes caused recompilation of the cached object).

platform/ios/keyboard_input_view.mm:187:27:{187:25-187:26}{187:29-187:87}: error: comparison of integers of different signs: 'NSInteger' (aka 'long') and 'unsigned long' [-Werror,-Wsign-compare,2]
                for (NSInteger i = 0; i < MIN([substringToDelete length], [substringToEnter length]); i++) {
                                       ~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@akien-mga
Copy link
Member

Looks good! Could you squash the commits? See PR workflow for instructions.

Co-authored-by: bruvzg <7645683+bruvzg@users.noreply.github.com>
@YYF233333
Copy link
Contributor Author

Sorry, left for a while. Squashed.

@akien-mga
Copy link
Member

akien-mga commented Dec 20, 2024

BTW, I'd be interested in knowing your workflow for identifying these unused headers.

I've tried to use iwyu for this exact use case in the past, but I could only get it to add includes that are used through existing explicit includes (which would bloat things considerably, as we tend to expect that including e.g. node_2d.h is sufficient and you don't need also explicit canvas_item.h, node.h, object.h, etc.). But indeed a "don't-include-what-you-don't-use" is sorely needed for C/C++ codebases.

Edit: Closed by mistake.

@akien-mga akien-mga closed this Dec 20, 2024
@akien-mga akien-mga reopened this Dec 20, 2024
@YYF233333
Copy link
Contributor Author

BTW, I'd be interested in knowing your workflow for identifying these unused headers.

Actually clangd is enough for identifying unused header. Everytime I open a file in vscode there are unused header warnings which is annoying and finally I decide to clean up them.

iwyu gives some more aggressive suggestions, like replace some headers with forward declarations.
e.g.

core/os/memory.h should add these lines:
#include "core/typedefs.h"            // for _FORCE_INLINE_, _ALWAYS_INLINE_
template <typename T> class SafeNumeric;

core/os/memory.h should remove these lines:
- #include <type_traits>  // lines 39-39
- #include "core/templates/safe_refcount.h"  // lines 35-35

I'm currently evaluating how much build time they can save, and may pick some big improvements (if there are any) without sacrificing readability.

@akien-mga akien-mga modified the milestones: 4.x, 4.4 Dec 20, 2024
@akien-mga akien-mga merged commit fd5548a into godotengine:master Dec 20, 2024
40 checks passed
@akien-mga
Copy link
Member

Thanks!

@YYF233333 YYF233333 deleted the iwyu2 branch December 21, 2024 02:44
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