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

Fix infinite recursion when printing errors #85137

Closed
wants to merge 102 commits into from

Conversation

ltecheroffical
Copy link

FIxes a tilemap bug in godot #84892.
Fixes as well an a potential infinite loop caused by repeating messages.

BastiaanOlij and others added 10 commits November 16, 2023 13:34
The parsed language parameters contain unstripped spaces. This will
generate a wrong path.

Provide a `TOOLSOPT` to allow overriding the default values of
parameters of the `make_rst.py` script.

The xml generated by `godot --doctool -l LANG` can be checked for
errors using `make xml-check LANGARG=LANG`, which may be useful for
checking errors in po files.
Also add an explicit way to trigger the tool manually
at user's will.
core/object/message_queue.cpp Show resolved Hide resolved
core/templates/self_list.h Outdated Show resolved Hide resolved
core/object/message_queue.cpp Show resolved Hide resolved
core/object/message_queue.cpp Show resolved Hide resolved
core/object/message_queue.cpp Outdated Show resolved Hide resolved
core/templates/self_list.h Outdated Show resolved Hide resolved
@AThousandShips AThousandShips dismissed their stale review November 20, 2023 15:23

Resolved (partially)

@AThousandShips AThousandShips changed the title Fix tilemap freezing godot when used Fix infinite recursion when printing errors Nov 20, 2023
ltecheroffical and others added 2 commits November 20, 2023 10:25
Co-authored-by: A Thousand Ships <[email protected]>
Co-authored-by: A Thousand Ships <[email protected]>
@@ -93,7 +95,7 @@ Error CallQueue::push_callablep(const Callable &p_callable, const Variant **p_ar

if ((page_bytes[pages_used - 1] + room_needed) > uint32_t(PAGE_SIZE_BYTES)) {
if (pages_used == max_pages) {
ERR_PRINT("Failed method: " + p_callable + ". Message queue out of memory. " + error_text);
printf((const char*)("Failed method: " + p_callable + ". Message queue out of memory. " + error_text).get_data());
Copy link
Member

@AThousandShips AThousandShips Nov 20, 2023

Choose a reason for hiding this comment

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

Also this cast isn't valid, it casts const char32_t * to const char *, I think it should be:

Suggested change
printf((const char*)("Failed method: " + p_callable + ". Message queue out of memory. " + error_text).get_data());
printf((const char*)("Failed method: " + p_callable + ". Message queue out of memory. " + error_text).utf8());

Copy link
Member

Choose a reason for hiding this comment

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

Passing dynamic value to the printf will cause warnings with some compilers (it's not safe since string can contain %*), use something like:

printf("%s\n", ("Failed method: " + p_callable + ". Message queue out of memory. " + error_text).utf8().get_data());

or

printf("Failed method: %s. Message queue out of memory. %s\n", String(p_callable).utf8().get_data(), error_text.utf8().get_data());

Copy link
Member

Choose a reason for hiding this comment

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

That does seem a better approach

Copy link
Member

Choose a reason for hiding this comment

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

Also this cast isn't valid, it casts const char32_t * to const char *, I think it should be:

It's .utf8().get_data() not just .utf8() (which returns CharStringobject).

Copy link
Member

@AThousandShips AThousandShips Nov 20, 2023

Choose a reason for hiding this comment

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

That's what the cast was for 🙂, it has:

operator const char *() const { return get_data(); };

But using get_data is more clear, but was just tweaking the existing code

@ltecheroffical ltecheroffical requested review from a team as code owners November 26, 2023 17:33
@AThousandShips
Copy link
Member

AThousandShips commented Nov 26, 2023

You need to use git push --force as the instructions say, and you didn't squash the changes into one

If you need help I can take this over, since all the added code has been instructed by others

@ltecheroffical
Copy link
Author

Yep it seems like you need to take this over. Somehow issues on my end.

@AThousandShips
Copy link
Member

AThousandShips commented Nov 26, 2023

Superseded by:

Thank you for your contribution nonetheless

@AThousandShips AThousandShips added archived and removed needs work cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Nov 26, 2023
@AThousandShips AThousandShips removed this from the 4.3 milestone Nov 26, 2023
@AThousandShips AThousandShips removed request for a team November 26, 2023 19:29
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.