Style: Purge .compat.inc files

[Repiteo]

I very, very strongly dislike the use of .compat.inc files. They exist as files dedicated to a handful of function implementations that aren't even wholly separated because they have to be initially declared in header files. From where I sit, it makes much more sense to handle these functions in their respective implementation file, as we're already doing in their headers. I don't have any concrete or objective resources to back this up, it's really as simple as me not liking them to the extent that I made a PR entirely out of spite

[KoBeWi]

These files allow for clearer separation of compatibility code.
I'd actually go in opposite direction and do more of that (#89160).

it makes much more sense to handle these functions in their respective implementation file, as we're already doing in their headers.

They are in the headers only because it's the only way.

[Repiteo]

They are in the headers only because it's the only way.

Ehh, not entirely. We've seen other headers use .inc files to define logic as if they were in headers; the most obvious example being bvh_tree.h. I'd be MUCH more open to the idea of these files if the headers got the same treatment. Hell, you could probably make dedicated #ifdef <HEADER_NAME_H> defines to have that same file handle header and implementation logic

...Shit, I might've talked myself into this

[akien-mga]

...Shit, I might've talked myself into this

Let me put a blocker here for now. This needs to be discussed with maintainers before doing more significant work there.

This kind of change shouldn't be done on a whim for personal taste reasons. There's a reason why we designed a system with separate files for compat code.
You would need to look up that reason before deciding to yank it.

Edit: I dug up through the history for this which wasn't obvious to find. This was designed in #76577, you'll need to expand some of the resolved discussions to see it.

I'm pretty sure there was more discussion spread across Rocket.chat and some issues, but yeah at this point tracking it down is a bit of investigative journalism work :P

But in short:

• We want a clear separation of concern between the classes' intended API, and the legacy compatibility code. Using a separate file for this is a great way to make it so that developers and users don't need to bother about this code if they're not themselves adding compat breakages. In some cases it's a significant amount of deprecated code that needs to be kept for compatibility, and we're only 2 minor releases into using that system, more will accumulate over time.

See e.g. these examples from this PR:

 scene/animation/animation_player.compat.inc   |  76 ---------
 scene/animation/animation_player.cpp          |  48 +++++-
 scene/gui/popup_menu.compat.inc               |  81 ----------
 scene/gui/popup_menu.cpp                      |  53 +++++-
 servers/rendering/rendering_device.compat.inc | 151 ------------------
 servers/rendering/rendering_device.cpp        | 123 +++++++++++++-

Adding 123 lines of code to rendering_device.cpp that are not relevant for the current workings of this class is just adding more code that developers need to ignore or somehow load in their mental model when working on the class (and this will keep increasing, there are more compat breakages in the pipeline, and some more complex to handle than just a oneliner redirection method).

• Eventually, if we move to 5.0, we'll want to drop all this compatibility code. Having it well self-contained in .compat.inc files is a nice way to find what code should be removed.

• .compat.inc files included in the .cpp were chosen to avoid increasing build time, so this is just injecting some code in the .cpp - we keep concerns separate but don't have to pay extra for it in build time.

• This system is now well established, documented, and I don't understand why it's so bad that it would need to be undone. This seems like a pointless refactoring to me.

• I don't claim the current system is perfect and can't be improved though - apologies for my strongly worded initial reaction. It mostly comes from thinking that we have higher priorities to tackle than refactoring things that work fine.

[akien-mga]

This goes against our agreed guidelines with no good objective reason.

[Repiteo]

You know what, that's entirely reasonable. I might not entirely agree, but their reason for existing being laid out makes a lot of things "click". Closing.