Bug 39950 – Documentation on various assertion macros

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jeremy Moskovich]

Per discussion in Bug 36527, it would be nice to have a page on the site detailing the various assertion macros and their use vs the crash macro.

[Jeremy Moskovich]

Created attachment 57455 [details]
Documentation on assertion macros

I tried to incorporate all the comments from the previous bug, hope I didn't miss anything.

[Darin Adler]

Comment on attachment 57455 [details]
Documentation on assertion macros

> +  <li>ASSERT(expression) - if expression returns false then abort execution and break into the debugger.

> +  <li>ASSERT_UNREACHED() - For use when a certain branch of code should never be executed.

> +  <li>ASSERT_UNUSED(variable, expression) - In release builds this compiles down to

> +  <li>ASSERT_ARG(variable, expression - same as ASSERT() but this prints <span class="variable">variable</span> as a string as part of the assertion output.

> +  <li>ASSERT_WITH_MESSAGE(expression, ...) - Accepts a printf() style variable number of arguments after the expression which are printed out if the assertion fails.

This is not a parallel construction. The description of ASSERT is a sentence that starts with a lowercase letter. The description of ASSERT_UNREACHED starts with "for use". The description of ASSER_UNUSED starts with "in release builds". The comments about each macro should use a parallel form so it's easy to understand.

For ASSERT_UNUSED I would say something like: "For assertions that check the value of otherwise unused function arguments."

This document does not make it clear when I should use ASSERT_ARG. For good reason. I don't know when we should use it instead of ASSERT. Maybe we should just not document it.

> +  THE CRASH() macro

The word "THE" should not be capitalized here.

> it's omission

Typo: Should be "its omission".

> +  <li>The ideal is to recover from errors, possibly after ASSERT()ing if the condition might indicate a programming error.
> +  </li>

I don't think this is quite right. I world use wording more like "When possible, the best design ..." or something like that. I'm not sure "The ideal is" will be understood by readers.

I think the document even it its current form will do more good than harm, so I am tempted to say review+

I also noticed that some of the code does not match our coding style. Specifically the space after the "}" and before the ";" in the enum. Also, the code example shows a switch statement with a default case, which is something we normally don't do because we want the GCC compiler to catch missing enum cases.

[Jeremy Moskovich]

Created attachment 57488 [details]
Documentation on assertion macros

Thanks Darin, hopefully all comments addressed.

I've gone with a simpler code example which hopefully illustrates the same point, let me know if you think there's one that would work better.

[Jeremy Moskovich]

Ping?  Does the new patch look ok to everyone?

[Darin Adler]

Comment on attachment 57488 [details]
Documentation on assertion macros

Another round of comments:

WebKitSite/coding/assertion-guidelines.html:35
 +    The ASSERT() macro and it's variants are compiled out of release builds. They are meant for use during the development process to catch programming mistakes. For those macros that accept an expression as an argument, the expression is also compiled out of release builds and thus incurs no overhead in shipping code.
The "it's" here should be "its".

WebKitSite/coding/assertion-guidelines.html:38
 +    <li>ASSERT(expression) - for use as a predicate that a condition defined by expression should not occur. If expression evaluates to false then abort execution and break into the debugger.
The grammar here is nonstandard. I don't think "a predicate that a condition should not occur" is correct grammar, and I'm confused by the use of both predicate and condition, which mean almost the same thing, to mean different things here. It's the expression that is a predicate, not the ASSERT macro so the macro is not for use "as a predicate". I suggest saying "If the expression" rather than "If expression".

WebKitSite/coding/assertion-guidelines.html:50
 +    <li>ASSERT_UNUSED(variable, expression) - for assertions that check the value of otherwise unused function arguments.
"check the value of an otherwise unused variable", not function

WebKitSite/coding/assertion-guidelines.html:54
 +  (void)variable
I think the use of (void) to silence the unused variable warning is an implementation detail that should be left out of the documentation.

WebKitSite/coding/assertion-guidelines.html:64
 +      ASSERT_UNUSED(exec, exec-&gt;hadException() &amp;&amp; exec-&gt;exception() == m_exception);
It's considered bad style to use && in an assertion. We normally split such assertions in two, so you can see which half failed.

WebKitSite/coding/assertion-guidelines.html:69
 +    <li>ASSERT_WITH_MESSAGE(expression, ...) - for use when you want to print extra information in the case of a failed assertion. Accepts a printf() style variable number of arguments after the expression which are printed out if the assertion fails.
I'm not sure we should document ASSERT_WITH_MESSAGE. It's rarely used and we might want to phase it out.

WebKitSite/coding/assertion-guidelines.html:82
 +    CRASH() raises a fatal error resulting in program termination and triggering either the debugger or the crash reporter. It is active in both debug &amp; release mode. CRASH() has a direct effect on end users and can be used in the field to gather information.
I think "used in the field" is unclear here. We use it in the source code, although its effect is felt "in the field". Please find a clearer way to word this.

WebKitSite/coding/assertion-guidelines.html:85
 +    Considerations when using ASSERT*() and CRASH() macros.
I think the use of ASSERT* to refer to the family of assertion macros is inelegant. Maybe something more like "the ASSERT family of macros" would be better? Not sure.

WebKitSite/coding/assertion-guidelines.html:91
 +    The expression inside the ASSERT(expression) macro is compiled out of release builds together with the macro itself. If the expression that's used has side effects, its omission in release build can lead to programming errors that don't manifest in debug builds.
I suggest calling this the ASSERT macro, rather than the ASSERT(expression) macro. "inside the ASSERT or ASSERT_UNUSED macro".

WebKitSite/coding/assertion-guidelines.html:94
 +    The benefits of a CRASH():
"The benefits of using CRASH:"

WebKitSite/coding/assertion-guidelines.html:106
 +    <li>Turns an internal programming error into a crash. Blows away at least a tab, and in some cases the entire web browser, in cases that otherwise might be harmless.
This is not grammatically consistent with the other list items. I think you should instead say "Use of the CRASH macro turns a programming error into a crash, blowing away a webpage or an entire web browser in cases that otherwise might be harmless."

WebKitSite/coding/assertion-guidelines.html:108
 +    <li>The cost of evaluating the expression that determines whether the error condition has occurred is incurred in release builds rather than being compiled out like it is with the ASSERT() macro.
The cost is not compiled out. The expression is compiled out. Please reword. I also think this does not state the cost clearly enough. Something like, "Checking for the error condition may slow the program down."

WebKitSite/coding/assertion-guidelines.html:115
 +    <li>Use ASSERT() for things that should never happen, but if they do will cause incorrect results, not a crash or memory corruption.
I would say "incorrect results rather than".

WebKitSite/coding/assertion-guidelines.html:117
 +    <li>Use CRASH() for cases that shouldn't happen, but if they do would be unrecoverable. e.g. OOM errors.
I suggest "out of memory" instead of "OOM".

WebKitSite/coding/assertion-guidelines.html:119
 +    <li>When possible, the best design is to recover from errors, possibly after ASSERT()ing if the condition might indicate a programming error.
I would say the "best practice is to include code that can recover from errors", but I am not sure this is the best practice. We don't always want code to recover from errors after ASSERT. Doing this everywhere would add many unneeded and untested code paths. This comment oversimplifies and does not make it clear what the best practice really is. We should rethink this point and figure out what to say.

The WebKitSite/coding/assertion-guidelines.html:129
 +  Object* b = new Object;
This example is wrong, because our operator new will never return a 0 because of memory exhaustion; new automatically calls CRASH for you. We need an example with a function that actually does return 0. Maybe tryFastMalloc or something else like that.

[Jeremy Moskovich]

Created attachment 58000 [details]
Documentation on assertion macros

Thanks Darin, all fixed. If you think I should rephrase other parts I'd be grateful for suggestions.

[Darin Adler]

Comment on attachment 58000 [details]
Documentation on assertion macros

I think there's one significant type of guideline missing here: Assertions should only be used for conditions that the programmer knows won't occur. We should never assert, for example, that a file system call has succeeded, because there is no way for a programmer to guarantee that. Assertions are claims that a programmer knows to be true, and they fire only when that programmer turns out to be wrong because there is some kind of programming mistake. There should be no "wishful thinking" assertions.

The phrase "other bad behavior deserving attention" fuzzes this issue a bit. There are many types of bad behavior where an assertion is not an acceptable way to check.

The section that talks about "recover from non-fatal errors" is also now vague enough that I think it's not as useful as it once was.

I'd prefer that the examples include the best programming style as possible. I'm not all that fond of the idea of naming a local variable "mem" in the example.

I think it's OK to land this now, and I think there is room for improvement too.

[WebKit Commit Bot]

Comment on attachment 58000 [details]
Documentation on assertion macros

Clearing flags on attachment: 58000

Committed r60783: <http://trac.webkit.org/changeset/60783>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.