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

GDCLASS synced by ending with "private:" #1292

Merged
merged 1 commit into from
Nov 8, 2023

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Nov 1, 2023

The current implementation of GDCLASS exits the define with the scope set to "public". This doesn't match the behavior utilized in the godot repository, so people working in the source repository or making a module might be confused by a differing style. The most obvious scenario is someone expecting to have their functions/values after GDCLASS to be implicitly private, as has normally been the case, but with gdextension they will be public. This has the potential to go unnoticed for a long stretch of time, as there's no warning for if something should be private; so a user could realistically continue coding as if the values were private, but as soon as that repo is passed on to someone else that guarantee is gone

The only area this change risks regression in is classes that assume an implicitly public scope after GDCLASS. To me, this is entirely worth it, as it's to the end of consistent behavior for all parties moving forward. In addition, unlike the prior public example, there will be warnings/errors for attempting to access a private function/value, so an affected user could immediately recognize and address the regression in a matter of seconds

The same appendment was given to GDEXTENSION_CLASS, mirroring the trailing style of similar defines in the godot repository. This won't actually change anything currently generated, as an explicit "public:" is always defined right after (inferring that the generator was setup expecting a private environment), but it'll be convenient moving forward to have that parity. In addition, the same line buffer found in GDCLASS was added, so if any future changes to this define take place, they won't risk shaking up the diff logs

• Matches implementation used by modules and godot itself
• Apply same to GDEXTENSION_CLASS, setup with same diff-friendly spacers as GDCLASS
@Repiteo Repiteo requested a review from a team as a code owner November 1, 2023 18:05
@dsnopek dsnopek added the bug This has been identified as a bug label Nov 2, 2023
@dsnopek dsnopek added this to the 4.x milestone Nov 2, 2023
@dsnopek
Copy link
Collaborator

dsnopek commented Nov 2, 2023

Thanks! This looks good to me :-)

The only area this change risks regression in is classes that assume an implicitly public scope after GDCLASS. To me, this is entirely worth it, as it's to the end of consistent behavior for all parties moving forward.

We usually treat differences between Godot and godot-cpp as bugs, and will usually fix them even if they break source compatibility in small ways. In this case, I personally think it's fine.

@dsnopek dsnopek merged commit d239312 into godotengine:master Nov 8, 2023
12 checks passed
@AThousandShips AThousandShips modified the milestones: 4.x, 4.2 Nov 8, 2023
@Repiteo Repiteo deleted the GDCLASS-style-sync branch November 8, 2023 17:04
@dsnopek
Copy link
Collaborator

dsnopek commented Nov 10, 2023

Cherry-picked for 4.1 in PR #1306

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This has been identified as a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using GDCLASS macro makes private class members public
3 participants