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

Add DEV_ASSERT and DEV_CHECK macros #53393

Merged
merged 1 commit into from
Oct 5, 2021
Merged

Conversation

lawnjelly
Copy link
Member

Change the existing DEV_ASSERT macro to be switched on and off by the DEV_ENABLED define. DEV_ASSERT breaks into the debugger as soon as hit.
Add error macros DEV_CHECK and DEV_CHECK_ONCE to add an alternative check that ERR_PRINT when a condition fails, again only enabled in DEV_ENABLED builds.

c++ DEV only asserts

N.B. - These macros will now be compiled into DEV_ENABLED (debug) builds only, they will be compiled out in release and release_debug builds. They therefore are particularly aimed at use particularly in bottleneck code (such as renderer, physics, or accessors), or if you are not sure whether a check warrants a CRASH_COND or runtime check, but you want to document the assertion and check code flow in debug builds.

Polarity / Naming

Happy to hear opinions regarding naming / polarity of these functions.

They are both positive logic functions, which in my experience is more common for asserts in programming than the negative logic of e.g. ERR_FAIL and CRASH_COND which is often used in Godot. I personally find the positive logic more natural but maybe that is due to previous experience.

Although Godot normally does everything possible to prevent crashing / halting, programmers will be expecting asserts to halt execution immediately in a debugger. This is why it is probably a good idea for us to have another method for recoverable errors - hence the DEV_CHECK_ONCE and DEV_CHECK.

Notes

  • I had already been using DEV_ASSERT in the portal renderer, these are now active in debug builds.
  • I have converted some of the asserts in the batching. Some of the existing checks probably aren't necessary to be compiled in unless investigating specific problems so I've left quite a few as they are.
  • I'll also be going through the BVH to add / replace asserts as necessary in a separate PR.
  • We can do something similar in 4.x when the builds have been modified:
    Unify tools/target build type configuration to disambiguate "debug" (used for both game debugging and engine dev tools) godot-proposals#3371
  • It makes sense to standardise the macro names in both I guess so anyone feel free to chip in. (We can of course also edit these names later with a search and replace if there is a change of opinion in 4.x, so it's not completely vital we get it bang on first time)

Change the existing DEV_ASSERT function to be switched on and off by the DEV_ENABLED define. DEV_ASSERT breaks into the debugger as soon as hit.
Add error macros DEV_CHECK and DEV_CHECK_ONCE to add an alternative check that ERR_PRINT when a condition fails, again only enabled in DEV_ENABLED builds.
@lawnjelly lawnjelly requested review from a team as code owners October 4, 2021 14:16
@Calinou Calinou added this to the 3.4 milestone Oct 4, 2021
@fire
Copy link
Member

fire commented Oct 4, 2021

I prefer the polarity of DEV_FAIL and DEV_FAIL_ONCE.

There's two ways of checking.

  1. early return
  2. regular

Since Godot uses early return. I expect this feature to also to match.

https://arne-mertz.de/2016/12/early-return/

// Example of early return 
Data getSomeData(Configuration const& config) {
  if (theConfigIsBad(config)) {
    return Data{"BadConfig"};
  }

  // Deleted some exception usage

  Data data = tryToParseSomethingFromNetwork();
  if(!data.makesSense()) {
    return Data{"parseError"};
  }

  data.addSomeInformation();
  data.beautify();
  return data;
}

@akien-mga
Copy link
Member

I think this is fine as is for the 3.x branch where "positive" asserts were already used. We can bikeshed on the master branch version :)

@akien-mga akien-mga merged commit d82c75a into godotengine:3.x Oct 5, 2021
@akien-mga
Copy link
Member

Thanks!

@lawnjelly
Copy link
Member Author

@fire just to be clear, and regarding 3.x only - the new DEV_ENABLED flag is only present in debug build, not in release_debug.

This means that the distinction between error handling and error checking is much more important. As a rule, we probably do not want to be doing error handling in a DEV only build. If you handle an error in DEV build, then you will likely get a crash in a release build, and I probably don't need to elaborate on why this can be a bad idea.

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