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

[Mark Rowe]

On Apr 19, 2014, at 13:36, 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.

How does using an #if obfuscate the control follow and make the method harder to maintain?

> 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.

How would using an #if 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());

I disagree with your assertion that the latter is clearer. It’s certainly different, but how is it an improvement?

- Mark

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