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

[Filip Pizlo]

The #if means that the code being guarded isn't indented. I believe that indenting control flow is a good idea. 

Also, using normal if's whenever it would compile and be semantically equivalent means that the code being protected would benefit from syntax/type/semantic checking even in a release build. 

I think of this as being analogous to our preference for static assertions: we use them whenever we can but resort to dynamic assertions when we can't. 

-Filip

> On Apr 20, 2014, at 1:22 PM, Mark Rowe <mrowe at apple.com> wrote:
> 
> 
>> 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/9c4eac1d/attachment.html>