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

Implement error macros that come with an error message, replacing ERR_EXPLAIN. #30893

Merged
merged 1 commit into from Aug 8, 2019

Conversation

profan
Copy link
Contributor

@profan profan commented Jul 28, 2019

Motivation

ERR_EXPLAIN will currently evaluate whatever you pass to it in full regardless of whether the accompanying assertion actually fails, this can be a problem if you have a debug build (or release_debug like the editor) and in a loop have an assert + an error message with some string concatenation in it.

Personally I ran into it when evaluating my new astar implementation, and realised I'd come to a 30-50% slowdown once I'd inserted ERR_EXPLAIN usage for nice error messages, even though no errors were occurring. (our_no_err here still using asserts, but no ERR_EXPLAIN)

image

Solution

The error macro and the error message should be consolidated, so that for error macros that only need the error message when the assertion actually fails (like ERR_FAIL_COND) only then evaluate the operand for the message then, leading to less performance ache from this.

And so, I've implemented alternatives to our current error macros that look like so:

  • ERR_FAIL_COND -> ERR_FAIL_COND_MSG etc.
// previously, always evaluated
ERR_EXPLAIN("x must be positive, was: " + itos(x));
ERR_FAIL_COND(x < 0);

// now becomes, rhs only evaluated when it fails
ERR_FAIL_COND_MSG(x < 0, "x must be positive, was: " + itos(x));

Finally

I'm making this a draft PR so I can get some feedback on the implementation (the duplication might not be great, if someone has ideas for a neater implementation, I'm all ears) before I potentially sweep through the codebase and replace all usages of ERR_EXPLAIN, all feedback welcome!

@profan profan changed the title implement error macros that come with an error message, replacing ERR_EXPLAIN. Implement error macros that come with an error message, replacing ERR_EXPLAIN. Jul 28, 2019
@Chaosus Chaosus added this to the 3.2 milestone Aug 3, 2019
@akien-mga
Copy link
Member

@reduz says:

that sounds like a very good idea, I started using ERR_EXPLAIN more and more and it makes more sense like in this PR and it can be emptied in debug

I agree that it's a good change, I just merged two PRs today adding ERR_EXPLAIN()s in Node code which might have a slight impact on debug performance, so replacing those by these new macros will be good.

Can you rebase to squash the commits? We can then merge this PR, and work on porting all existing uses of ERR_EXPLAIN to these new macros (and eventually add more verbose errors in places where we only use ERR_FAIL).

@profan
Copy link
Contributor Author

profan commented Aug 8, 2019

Sure, squashed! 👍

@profan profan marked this pull request as ready for review August 8, 2019 13:07
@akien-mga akien-mga merged commit 22b42c3 into godotengine:master Aug 8, 2019
@akien-mga
Copy link
Member

Thanks!

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.

None yet

4 participants