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 explain message not being stripped in release. #33595

Merged
merged 1 commit into from
Nov 13, 2019

Conversation

Faless
Copy link
Collaborator

@Faless Faless commented Nov 13, 2019

Messages coming from ERR_EXPLAIN / ERR_*_MSG macros used to strip the error explanation in release builds and was changed in a recent refactoring.

This commit restores the old behaviour (fixing release builds).

Regression from #33517

Bugsquad edit: Fixes #33600.

Messages coming from ERR_EXPLAIN / ERR_*_MSG macros used to strip the
error explanation in release builds and was changed in a recent
refactoring.

This commit restores the old behaviour (fixing release builds).
@Faless Faless added this to the 3.2 milestone Nov 13, 2019
@akien-mga
Copy link
Member

CC @madmiraal

@madmiraal
Copy link
Contributor

I think I'm missing something. Why is stripping the error explanation in release builds the intended behaviour? Surely we would want the error explanation in the release builds too.

From what I can work out, it appears as if it's not stripped, just replaced. If the error message/explanation is an empty string, the logger code replaces it with the default error string here:

godot/core/io/logger.cpp

Lines 70 to 77 in d3a852f

const char *err_details;
if (p_rationale && *p_rationale)
err_details = p_rationale;
else
err_details = p_code;
logf_error("%s: %s\n", err_type, err_details);
logf_error(" At: %s:%i:%s() - %s\n", p_file, p_line, p_function, p_code);

Are there additional loggers that are using an empty string message as a proxy to decide not to print anything?

@Calinou
Copy link
Member

Calinou commented Nov 13, 2019

Why is stripping the error explanation in release builds the intended behaviour? Surely we would want the error explanation in the release builds too.

This is done to improve runtime performance (as assertions can be skipped) and decrease binary size (as messages aren't included in the binary) 🙂

@akien-mga
Copy link
Member

Indeed, ignoring those on release means that we don't need to evaluate the Strings in ERR_*_MSG macros at all, which can often involve complex operations (as examplified by #33600).

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.

build fails: error C3861: 'format_error_message': identifier not found
4 participants