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

Make sure to inizialize the r_error.error on Variant::construct #43535

Closed
wants to merge 1 commit into from

Conversation

AndreaCatania
Copy link
Contributor

Make sure to initialize the r_error.error when Variant::construct is called.

The error passed to the function Variant::construct is not initialized: https://github.com/godotengine/godot/blob/master/modules/gdscript/gdscript_function.cpp#L958-L959

Since not all the constructors defined here: https://github.com/godotengine/godot/blob/master/core/variant/variant_construct.cpp#L738 always set r_error.error to something, happens that the error Bug, call error: #426275124 is raised with a random number, even when the function succeed.

Correct functions:

Bad functions:

If we initialize the variable to CALL_OK, as I did in this PR, we can just leave the task to the above functions to set the error only when there is an error. In this way we can also make that mechanism less error prone and so more robust. Though, not sure if you would like to also set CALL_OK into those functions, or just leave them as are @reduz.

@vnen
Copy link
Member

vnen commented Nov 23, 2020

I believe we only need to initialize this in the CallError constructor (which is done among other things in #43730). This way we don't need to initialize on every call.

@AndreaCatania
Copy link
Contributor Author

Yes, it works too.

@akien-mga
Copy link
Member

Superseded by #43730.

@akien-mga akien-mga closed this Nov 24, 2020
@AndreaCatania AndreaCatania deleted the variant_fix branch November 28, 2020 12:58
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.

4 participants