<html><head><meta http-equiv="content-type" content="text/html; charset=utf-8"></head><body dir="auto"><div>The #if means that the code being guarded isn't indented. I believe that indenting control flow is a good idea. </div><div><br></div><div>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. </div><div><br></div><div>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. <br><br>-Filip</div><div><br>On Apr 20, 2014, at 1:22 PM, Mark Rowe <<a href="mailto:mrowe@apple.com">mrowe@apple.com</a>> wrote:<br><br></div><blockquote type="cite"><div><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><br><div><div>On Apr 19, 2014, at 13:36, Filip Pizlo <<a href="mailto:fpizlo@apple.com">fpizlo@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; margin: 0px;">Here are some examples.</div><div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; margin: 0px;"><br></div><div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; margin: 0px;">Guarding a loop that does assertions:</div><div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; margin: 0px;"><br></div><div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; margin: 0px;"><div id="bloop_customfont" style="margin: 0px;"> if (!ASSERT_DISABLED) {</div><div id="bloop_customfont" style="margin: 0px;"> for (unsigned index = 0; index < m_generationInfo.size(); ++index) {</div><div id="bloop_customfont" style="margin: 0px;"> GenerationInfo& info = m_generationInfo[index];</div><div id="bloop_customfont" style="margin: 0px;"> RELEASE_ASSERT(!info.alive());</div><div id="bloop_customfont" style="margin: 0px;"> }</div><div id="bloop_customfont" style="margin: 0px;"> }</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">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.</div></div></blockquote><div><br></div>How does using an #if obfuscate the control follow and make the method harder to maintain?</div><div><br><blockquote type="cite"><div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; margin: 0px;"><div>Guarding code in the JIT that emits an assertion:</div><div><br></div><div><div> if (!ASSERT_DISABLED) {</div><div> JITCompiler::Jump ok = m_jit.branch32(</div><div> JITCompiler::GreaterThanOrEqual, allocatorGPR, TrustedImm32(0));</div><div> m_jit.breakpoint();</div><div> ok.link(&m_jit);</div><div> }</div></div><div><br></div><div>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.</div></div></blockquote><div><br></div><div>How would using an #if make the code harder to maintain?</div><div><br></div><blockquote type="cite"><div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; margin: 0px;"><div>Counterexample where we using an #if to guard an assertion in the JIT:</div><div><br></div><div><div>#if !ASSERT_DISABLED</div><div> SpeculateCellOperand op1(this, node->child1());</div><div> JITCompiler::Jump isOK = m_jit.branchStructurePtr(</div><div> JITCompiler::Equal, </div><div> JITCompiler::Address(op1.gpr(), JSCell::structureIDOffset()), </div><div> node->structure());</div><div> m_jit.breakpoint();</div><div> isOK.link(&m_jit);</div><div>#else</div><div> speculateCell(node->child1());</div><div>#endif</div></div><div><br></div><div>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:</div><div><br></div><div><div> if (!ASSERT_DISABLED) {</div><div> SpeculateCellOperand op1(this, node->child1());</div><div> JITCompiler::Jump isOK = m_jit.branchStructurePtr(</div><div> JITCompiler::Equal, </div><div> JITCompiler::Address(op1.gpr(), JSCell::structureIDOffset()), </div><div> node->structure());</div><div> m_jit.breakpoint();</div><div> isOK.link(&m_jit);</div><div> } else</div><div> speculateCell(node->child1());</div></div></div></blockquote><div><br></div><div>I disagree with your assertion that the latter is clearer. It’s certainly different, but how is it an improvement?</div><div><br></div><div>- Mark</div><div><br></div></div></div></blockquote></body></html>