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.

Sign up for GitHub

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

Jump to bottom

GDScript: Check get_node() shorthand in static functions #78552

Merged
merged 1 commit into from
Aug 17, 2023
Merged

GDScript: Check get_node() shorthand in static functions #78552

merged 1 commit into from
Aug 17, 2023

Conversation

dalexeev
Copy link
Member

@dalexeev dalexeev commented Jun 22, 2023 •
edited
Loading

@dalexeev dalexeev requested a review from a team as a code owner June 22, 2023 09:20
@akien-mga akien-mga added this to the 4.2 milestone Jun 22, 2023
@adamscott adamscott added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jun 22, 2023
adamscott
adamscott approved these changes Jun 22, 2023
View reviewed changes
Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me! Thanks @dalexeev for the PR and the test!

@dalexeev
Copy link
Member Author

dalexeev commented Jun 22, 2023 •
edited
Loading

Perhaps we should move use_dollar out of the DEBUG_ENABLED (here and elsewhere) to simplify the code a bit. I don't think it has a noticeable impact on performance and memory consumption. Especially if we bring back support for bytecode resources (.gdc) in the future.

@adamscott
Copy link
Member

Perhaps we should move use_dollar out of the DEBUG_ENABLED (here and elsewhere)

Added the PR to the list of PRs for the discussion about that.

@vnen
Copy link
Member

vnen commented Aug 9, 2023

Perhaps we should move use_dollar out of the DEBUG_ENABLED (here and elsewhere) to simplify the code a bit.

It should be moved out. Especially because error messages should not differ between debug and release, because we can still run the tests with release builds and this difference would create a false negative.

@dalexeev dalexeev force-pushed the gds-check-get-node-in-static-func branch from 58fc352 to 0f27c4a Compare August 9, 2023 14:11
@dalexeev
Copy link
Member Author

dalexeev commented Aug 9, 2023

It should be moved out. Especially because error messages should not differ between debug and release

Done.

@akien-mga akien-mga merged commit 4f00f92 into godotengine:master Aug 17, 2023
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@dalexeev dalexeev deleted the gds-check-get-node-in-static-func branch August 17, 2023 10:19
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.3.

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamscott adamscott adamscott approved these changes

@akien-mga akien-mga akien-mga approved these changes

Assignees
No one assigned
Labels
bug topic:gdscript
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.

Accessing scene unique nodes in a static function from a different node causes error instead of returning null
5 participants
@dalexeev @adamscott @vnen @akien-mga @YuriSizov