Style: Add class_db.h includes explicitly#116826
Conversation
There was a problem hiding this comment.
I'm almost surprised to say but I'm fine with the class_db.h explicit include.
It's a great approach: Just add it to all the implementation files of Object subclasses. Makes for a real simple PR, and one that unlocks us from removing the include from headers much easier.
I'm not 100% sure about the .compat.inc. Both positions come with potential downsides: If it's at the start it cannot use the .cpp file's static functions, if it's at the end, the .cpp files cannot use compat functions (though perhaps they shouldn't need to?). Overall I don't have a strong preference for this which is why I haven't made an official proposal to change this so far. Maybe it would be ideal to actually have it be a separate .cpp file altogether, and one that only affects the header by declaring like _bind_compat_functions or something like that. But I'm not sure it's worth the churn to change it at all right now?
Anyway, enough rambling, i'd say it at least needs some separate discussion.
|
If the |
4d31f8f to
5b2deb8
Compare
class_db.h includes explicitly; migrate *.compat.inc includes to bottom of fileclass_db.h includes explicitly
|
Opted for the #116737 approach instead, where the |
class_db.hinclude fromref_counted.h#116737class_db.hinclude fromresource.h#111573During yesterday's core meeting, several topics were touched on with regards to the process by which we should be removing implicit includes for the comically-prevalent
class_db.h. The current consensus seems to be that it's something that allObjectsubclasses rely on, so it would result in a fair amount of churn for the codebase when rolling out those changes. Tracking performance changes would also be tricky, as the number of files that actually need the include is hard to get a concrete answer for, even with the two above PRs in isolation (combined might paint a different story).This stylistic PR is made with the intent of getting the churn itself over with as quickly and painlessly as possible. For every single implementation file that binds in some capacity, this PR adds
#include "core/object/class_db.hexplicitly. There are zero functional changes as a result of this PR, because every single one of these files requires this include to begin with. As such, this PR similar to #116539 (albeit much wider in scope), where it more clearly spells out the "problem" includes by going against the minimal-include default.On a somewhat-related note: compatibility include order was brought up during theEDIT: In the interest of keeping this PR free of blockers, I've instead implemented the #116737 approach of addingClassDBcore meeting topic, as it was discovered in #116737 that we cannot simply putclass_db.hafter*.compat.inc. Ivorforce suggested that we could just drop all those includes to the bottom of the file instead, something thatgodot-cppalready does. Since I'm already touching literally every single file that has those compatibility includes, as they're only a concept forObjectsubclass bindings, I tackled their migration in the same pass. This too resulted in zero functional changes, so it's similarly ignored within the same commit.class_db.hincludes to the*.compat.incfiles instead