[webkit-dev] Style proposal for branch style for macros

[Geoffrey Garen]

Seems like an improvement to me.

Two other things that would improve this code for me:

(1) “Not doing no assertions” hurts my brain. I wish we could use “if (ASSERT_ENABLED)” instead.

(2) ASSERT is easier to understand if it only adds code, and does not remove code. So, in the “ASSERT else speculateCellI()” code, I’d prefer to make the speculateCell() unconditional. That’s easier to read. It’s true that the speculateCell() is redundant with the previous ASSERT, but there’s no harm done by that.

Geoff

On Apr 19, 2014, at 1:36 PM, Filip Pizlo <fpizlo at apple.com> wrote:

> Here are some examples.
> 
> Guarding a loop that does assertions:
> 
>     if (!ASSERT_DISABLED) {
>         for (unsigned index = 0; index < m_generationInfo.size(); ++index) {
>             GenerationInfo& info = m_generationInfo[index];
>             RELEASE_ASSERT(!info.alive());
>         }
>     }
> 
> The one above comes from DFGSpeculativeJIT.cpp, and it is in a non-trivial method.  Using an #if here would obfuscate the control flow and make the method harder to maintain.  Arguably, a ASSERT would have been fine because the compiler "should" be smart enough to kill the loop if the ASSERT is a no-op.  I'm not sure all compilers would be smart enough, though - particularly if m_generationInfo is any kind of interesting collection class.
> 
> Guarding code in the JIT that emits an assertion:
> 
>     if (!ASSERT_DISABLED) {
>         JITCompiler::Jump ok = m_jit.branch32(
>             JITCompiler::GreaterThanOrEqual, allocatorGPR, TrustedImm32(0));
>         m_jit.breakpoint();
>         ok.link(&m_jit);
>     }
> 
> The one above also comes from the DFG JIT.  We have lots of code like this.  The assertions are very useful but obviously we don't want them in a release build.  Using an #if would make the code harder to maintain.
> 
> Counterexample where we using an #if to guard an assertion in the JIT:
> 
> #if !ASSERT_DISABLED
>         SpeculateCellOperand op1(this, node->child1());
>         JITCompiler::Jump isOK = m_jit.branchStructurePtr(
>             JITCompiler::Equal, 
>             JITCompiler::Address(op1.gpr(), JSCell::structureIDOffset()), 
>             node->structure());
>         m_jit.breakpoint();
>         isOK.link(&m_jit);
> #else
>         speculateCell(node->child1());
> #endif
> 
> Code like this is harder to understand, especially if the code being guarded is longer than a page.  I don't think we should encourage this style, when it would have been clearer to do:
> 
>         if (!ASSERT_DISABLED) {
>             SpeculateCellOperand op1(this, node->child1());
>             JITCompiler::Jump isOK = m_jit.branchStructurePtr(
>                 JITCompiler::Equal, 
>                 JITCompiler::Address(op1.gpr(), JSCell::structureIDOffset()), 
>                 node->structure());
>             m_jit.breakpoint();
>             isOK.link(&m_jit);
>         } else
>             speculateCell(node->child1());
> 
> The point of coding conventions is to encourage clarity and common sense.  I don't think that #if's improve clarity.  I think what I'm proposing is a common sense rule that is easy to follow.  I trust everyone to know when a normal if statement would have been semantically equivalent to an #if statement.
> 
> -Phil
> 
> 
> On April 19, 2014 at 1:22:23 PM, Geoffrey Garen (ggaren at apple.com) wrote:
> 
>> Can you give a motivating example from WebKit?
>> 
>> Geoff
>> 
>> On Apr 19, 2014, at 1:09 PM, Filip Pizlo <fpizlo at apple.com> wrote:
>> 
>>> Hey everyone,
>>> 
>>> When guarding code with macros that are always defined, such as ASSERT_DISABLED (it's always either 0 or 1), we have a choice between:
>>> 
>>> if (!ASSERT_DISABLED) {
>>>     // do things
>>> }
>>> 
>>> and:
>>> 
>>> #if !ASSERT_DISABLED
>>> // do things
>>> #endif
>>> 
>>> I'd like to propose that anytime the normal if would be semantically equivalent to the preprocessor #if, the normal if should be used.
>>> 
>>> We don't lose any compiler optimization, since even at -O0, the compiler will constant fold the normal if.  We do gain a lot of clarity, since the control flow of normal if statements is subject to proper indentation.
>>> 
>>> The "semantically equivalent" requirement still allows for #if to be used for thinngs like:
>>> 
>>> - Guarding the placement of fields in a class.
>>> - Guarding the definitions of other macros (in the case of ASSERT_DISABLED, we define ASSERT in different ways guarded by #if's)
>>> - Guarding the definition/declaration/inclusion of entire functions, classes, and other top-level constructs.
>>> 
>>> Thoughts?
>>> 
>>> -Phil
>>> 
>>> _______________________________________________
>>> webkit-dev mailing list
>>> webkit-dev at lists.webkit.org
>>> https://lists.webkit.org/mailman/listinfo/webkit-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-dev/attachments/20140420/648f5715/attachment.html>