[webkit-reviews] review denied: [Bug 39950] Documentation on various assertion macros : [Attachment 57488] Documentation on assertion macros

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Jeremy Moskovich
<playmobil at google.com>'s request for review:
Bug 39950: Documentation on various assertion macros
https://bugs.webkit.org/show_bug.cgi?id=39950

Attachment 57488: Documentation on assertion macros
https://bugs.webkit.org/attachment.cgi?id=57488&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.