Skip to content

Style: Remove redundant DEBUG_METHODS_ENABLED macro#106456

Merged
Repiteo merged 1 commit into
godotengine:masterfrom
Repiteo:style/remove-DEBUG_METHODS_ENABLED
May 19, 2025
Merged

Style: Remove redundant DEBUG_METHODS_ENABLED macro#106456
Repiteo merged 1 commit into
godotengine:masterfrom
Repiteo:style/remove-DEBUG_METHODS_ENABLED

Conversation

@Repiteo
Copy link
Copy Markdown
Contributor

@Repiteo Repiteo commented May 15, 2025

The macro DEBUG_METHODS_ENABLED is entirely redundant, serving no niche that isn't already fulfilled by the more ubiquitous and always-available DEBUG_ENABLED. This PR removes the former completely, because there's no risk of breaking backwards compatibility; A 1-to-1 replacement already exists with DEBUG_ENABLED, so anything using DEBUG_METHODS_ENABLED could just use DEBUG_ENABLED instead. Also adds // DEBUG_ENABLED to all #endif statements in the files that needed to be changed, because this is already a style pass so why not

• Replaced with functionally identical and far more ubiquitous `DEBUG_ENABLED`
@Repiteo Repiteo added this to the 4.x milestone May 15, 2025
@Repiteo Repiteo requested review from a team as code owners May 15, 2025 18:40
@Ivorforce
Copy link
Copy Markdown
Member

Getting rid of a duplicate macro makes sense, but what was DEBUG_METHODS_ENABLED introduced for?

@Repiteo
Copy link
Copy Markdown
Contributor Author

Repiteo commented May 15, 2025

I'm not certain what niche it filled. It's existed since the engine became open-source, and its instantiation was exactly the same. The only difference is it was in method_bind.h instead of typedefs.h, which it moved to in #42826. I see no reason to keep it around, as even if it was specifically for methods, that's still been supplanted by DEBUG_ENABLED

@Ivorforce
Copy link
Copy Markdown
Member

Okay, so it's possibly just a remnant of pre open source godot. Makes sense to remove it if it doesn't have a niché.

@akien-mga
Copy link
Copy Markdown
Member

Well one reason to potential keep this, is that it bears a different meaning to DEBUG_ENABLED, even if it's always enabled in the same case. It's about providing more debugging information in method bindings IIRC.

By removing this distinction, we're losing some intentionality there in what code gets flagged with DEBUG_METHODS_ENABLED specifically. Knowing it could open the way to e.g. enabling DEBUG_METHODS_ENABLED in release builds (with a SCons option) without having to enable DEBUG_ENABLED for the whole runtime.

That being said, we haven't done that in 10 years, and worst case we can always revert this PR if it was wanted in the future. I'm also not sure that new contributors after Juan have understood the distinction between DEBUG_ENABLED and DEBUG_METHODS_ENABLED and used it consistently where it should be.

@Repiteo
Copy link
Copy Markdown
Contributor Author

Repiteo commented May 16, 2025

If it were to be kept, we would need a dedicated sweep across the repo of what methods/variables should be using the "correct" macro. That feels like more trouble than it's worth, especially if we don't currently have a need for debug methods outside of debug builds

@Repiteo Repiteo modified the milestones: 4.x, 4.5 May 16, 2025
@Repiteo Repiteo merged commit 2bf7ac7 into godotengine:master May 19, 2025
20 checks passed
@Repiteo Repiteo deleted the style/remove-DEBUG_METHODS_ENABLED branch May 19, 2025 13:04
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.

3 participants