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

[Filip Pizlo]

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/20140419/1d52f772/attachment.html>