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

Style: Begin integrating simple clangd fixes #103075

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Feb 20, 2025

In contrast to the above PR, this aims to focus almost exclusively on basic header-related changes identified via clangd. So while a similar amount of files were touched, the actual functionality should be effectively identical & help address our header include issues. The only two non-header changes were replacing _DefaultComparator with Comparator (dependant files can now include core/typedefs.h instead of core/templates/sort_array.h) & replacing __aligned with the native alignas (fits the redundancy-removal theme).

@Repiteo Repiteo added this to the 4.5 milestone Feb 20, 2025
@Repiteo Repiteo requested review from a team as code owners February 20, 2025 15:11
@Repiteo Repiteo removed request for a team February 20, 2025 15:11
@Repiteo Repiteo removed request for a team February 20, 2025 15:11
@Repiteo Repiteo force-pushed the style/clangd-simple branch from 158cb1f to 473d6a3 Compare March 8, 2025 17:24
@Repiteo Repiteo requested a review from a team as a code owner March 8, 2025 17:24
@Repiteo Repiteo force-pushed the style/clangd-simple branch from 473d6a3 to 3e167e3 Compare March 11, 2025 17:45
@Repiteo Repiteo removed the request for review from a team March 11, 2025 17:45
@Repiteo Repiteo force-pushed the style/clangd-simple branch from 3e167e3 to cebc140 Compare March 12, 2025 19:26
Copy link
Member

@Ivorforce Ivorforce left a comment

Choose a reason for hiding this comment

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

This is nice, I'm looking forward to saner includes.
All the include changes I understood look sane. I can't guarantee correctness for every single one, but to be honest, if it compiles (on all platforms), and is less broad, it's good. Forward declare instead of include is especially appreciated.
Just one concern about the extern "C" change.

@Repiteo Repiteo force-pushed the style/clangd-simple branch from cebc140 to 11cc8a4 Compare March 22, 2025 17:42
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@@ -39,7 +39,7 @@
#include "core/string/translation_server.h"
#include "core/string/ucaps.h"
#include "core/variant/variant.h"
#include "core/version_generated.gen.h"
#include "core/version.h"
Copy link
Member

Choose a reason for hiding this comment

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

Not strongly opposed to it, but why that change? The only macro used is from version_generated.gen.h so it's more specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thinking was the export pragma in version.h makes it so that file is "exposing" the generated file, so it can be used in its place similar to using typedef.h instead of specific std headers. That's not as easily conveyed though, so I'm fine reverting back to the generated file.

Copy link
Member

@akien-mga akien-mga Mar 22, 2025

Choose a reason for hiding this comment

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

Well it's not a bad idea, and the cost on compilation time should be similar either way.

As long as we keep version_hash.gen.h separate (which changes for every commit, and causes quite a bit of CI churn as we can't fully reuse the cache), it should be fine.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good overall.

@Repiteo Repiteo force-pushed the style/clangd-simple branch from 11cc8a4 to f09ee01 Compare March 22, 2025 18:25
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