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 ERR_MSG macros to make their behaviour consistent with normal ERR macros. #32282

Closed
wants to merge 1 commit into from

Conversation

profan
Copy link
Contributor

@profan profan commented Sep 23, 2019

As seen in #32276, #31832, #32143, the current ERR_MSG macros sometimes seem to produce the error message provided and sometimes just the assert condition, apparently like in #32143 it also seems to differ between editor and the terminal..

This is a bandaid fix (as something else seems to cause the discrepancy, so if the error list system is used in another place, it'll still behave erratically in those cases), but this should at least make the behaviour consistent for error macro usage with and without a message.

Testing needed, CC @Xrayez @KoBeWi @qarmin mind helping out? :D

@Xrayez
Copy link
Contributor

Xrayez commented Sep 23, 2019

Yeah it shows up in editor now, as well as terminal, so alright:

png_fail_ok_editor

@Chaosus Chaosus added this to the 3.2 milestone Sep 23, 2019
@akien-mga
Copy link
Member

This change would make it harder to re-add the display of the actual C++ condition though, since you're no longer passing it to the printer methods.

@profan
Copy link
Contributor Author

profan commented Sep 24, 2019

@akien-mga The previous solution was just having two messages passed, I feel like it could be solved better by changing the error print function itself (an optional parameter maybe?).

Either way in the status quo the C++ condition is still not there (even without this PR), unless you somehow explicitly peek at the previous error message no?

I was actually going through #21884 and the changes done there but couldn't actually see easily how the error information was bundled before 🤔

I was basically figuring merge this first, then I make another PR which specifically solves bundling assert condition with the error message (like by making the message parameter a struct instead of just a char*), because having working error information for now is good and the second PR might take a little more time to get right.

But if you prefer that I just do the second PR and leave the current behaviour as it is (closing this PR), that's fine too 👀

@KoBeWi
Copy link
Member

KoBeWi commented Sep 24, 2019

I can confirm that it works in my case.

@akien-mga
Copy link
Member

This change would make it harder to re-add the display of the actual C++ condition though, since you're no longer passing it to the printer methods.

Re-adding the C++ condition display was trivial, as it was still part of the error message as expected: #32333.

I think this PR would break that (not tested though).

BTW, ERR_FAIL_*INDEX_MSG doesn't work, because the print function used for *INDEX* doesn't take the error message into account.

@profan
Copy link
Contributor Author

profan commented Sep 25, 2019

Oh yeah, I will close this PR then, thanks for looking into it @akien-mga (sorry I pushed a half-baked PR) :P

Slightly better understanding of the error system now at least 🤔

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.

5 participants