I'm surprised object.h doesn't include ClassDB (anymore?).

Doesn't that mean that any Object that wants to register properties with ADD_PROPERTY now needs to include ClassDB, which is far from obvious?

If that's the only coupling it does make sense to remove it of course, but then do we need to go one step further and turns ADD_PROPERTY and friends into member methods which are implemented in object.cpp (including class_db.h), so callers don't need to include it?

That exact confusion is why I'm wanting to migrate the macros to class_db.h instead:

• Core: Decouple ClassDB macros from object.h #110939

I will note that an explicit ClassDB include in compilation files is the paradigm that GDExtension uses, so I don't think it's inherently harmful. But it isn't the most obvious include either, so making them member functions instead could work out

(Oof, GH failed to send my comment, need to retype it all again...)

So this one warrants discussion. It may be worth it, but it's a bit of an anti-pattern IMO to forward-declare a class that it returns by a public method of a class (here MainLoop *OS::get_main_loop() and void OS::set_main_loop(MainLoop *p_main_loop).

It means that any class that uses this API from OS not only needs to include core/os/os.h, but also core/os/main_loop.h. That's unconventional IMO, and leads to surprises, as it seemed to be the case for you:

The latter was inconsequential, but the former revealed several dependency chains for core/os/main_loop.h, despite neither it nor the compilation file using MainLoop in any way.

Those compilation files all use MainLoop via OS::get_singleton()->get_main_loop().

Hmm... I'm not sure if I agree with it being an anti-pattern. If this was the only thing this class did then maybe, but not every file that calls OS is doing so for MainLoop.

They led to surprises for me because the implicit link is now broken, but that's an inevitability with this type of change. If we're wanting to move more towards including files with intent, then weeding out those that aren't is the price we gotta pay

I agree that it's not optimal. Ideally C++ wouldn't put us in a position where we choose between usability of an interface and compile time, but unfortunately that's just how the language is. Like Repiteo, I don't consider it an anti-pattern, just an inconvenience.

Besides needing to omit includes in some locations for technical reasons (cyclic dependencies), we have a problem of extremely long include chains in some spots. We will, I'm fully convinced, make the trade-off of requiring an additional include in some locations. From my perspective, this is a matter of judging each situation on a case by case basis.

In this specific instance, I'd say that it's worth it, because OS is a kitchen sink class, where only a handful of users will interface with the MainLoop class.

Should we make it a policy to document such incomplete return types with some comment that shows up in IDEs? What syntax gets picked up, /// comments?

To further illustrate my point, given the platform-related changes in this PR, I fully expect that this will cause a compilation error for third-party platform ports (e.g. consoles, homebrews) when they update. It could also potentially break a first-party platform we don't test on CI, though I think that's only visionOS and its calls to OS::get_main_loop are all shared with iOS through drivers/apple_embedded.

But yeah, I agree that this is worth doing for OS in this case. It's indeed a very commonly included class while only a handful of includers need to call get_main_loop(), so that's a good rationale for making it an incomplete definition.

Note that the title of this commit/PR is a bit misleading, as the biggest impact of this PR is the change to core/os/os.h which is not in core/config.

I think it's a bit superfluous, but there's no real harm in it. Adjusted to the // NOTE: syntax specified in the docs

Do // comments get picked up as documentation tooltips in IDEs?

It does in CLion.

It can in VSCode, though that technically depends on the extension. Both the Microsoft C/C++ and clangd extensions support it, which should cover most relevant cases

Sounds good, then I prefer the // Note: style too, Doxygen subjectively doesn't look very nice to me :P