<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Apr 19, 2014, at 13:36, Filip Pizlo &lt;<a href="mailto:fpizlo@apple.com">fpizlo@apple.com</a>&gt; 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;">&nbsp; &nbsp; if (!ASSERT_DISABLED) {</div><div id="bloop_customfont" style="margin: 0px;">&nbsp; &nbsp; &nbsp; &nbsp; for (unsigned index = 0; index &lt; m_generationInfo.size(); ++index) {</div><div id="bloop_customfont" style="margin: 0px;">&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; GenerationInfo&amp; info = m_generationInfo[index];</div><div id="bloop_customfont" style="margin: 0px;">&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; RELEASE_ASSERT(!info.alive());</div><div id="bloop_customfont" style="margin: 0px;">&nbsp; &nbsp; &nbsp; &nbsp; }</div><div id="bloop_customfont" style="margin: 0px;">&nbsp; &nbsp; }</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. &nbsp;Using an #if here would obfuscate the control flow and make the method harder to maintain. &nbsp;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. &nbsp;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>&nbsp; &nbsp; if (!ASSERT_DISABLED) {</div><div>&nbsp; &nbsp; &nbsp; &nbsp; JITCompiler::Jump ok = m_jit.branch32(</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; JITCompiler::GreaterThanOrEqual, allocatorGPR, TrustedImm32(0));</div><div>&nbsp; &nbsp; &nbsp; &nbsp; m_jit.breakpoint();</div><div>&nbsp; &nbsp; &nbsp; &nbsp; ok.link(&amp;m_jit);</div><div>&nbsp; &nbsp; }</div></div><div><br></div><div>The one above also comes from the DFG JIT. &nbsp;We have lots of code like this. &nbsp;The assertions are very useful but obviously we don't want them in a release build. &nbsp;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>&nbsp; &nbsp; &nbsp; &nbsp; SpeculateCellOperand op1(this, node-&gt;child1());</div><div>&nbsp; &nbsp; &nbsp; &nbsp; JITCompiler::Jump isOK = m_jit.branchStructurePtr(</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; JITCompiler::Equal,&nbsp;</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; JITCompiler::Address(op1.gpr(), JSCell::structureIDOffset()),&nbsp;</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; node-&gt;structure());</div><div>&nbsp; &nbsp; &nbsp; &nbsp; m_jit.breakpoint();</div><div>&nbsp; &nbsp; &nbsp; &nbsp; isOK.link(&amp;m_jit);</div><div>#else</div><div>&nbsp; &nbsp; &nbsp; &nbsp; speculateCell(node-&gt;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. &nbsp;I don't think we should encourage this style, when it would have been clearer to do:</div><div><br></div><div><div>&nbsp; &nbsp; &nbsp; &nbsp; if (!ASSERT_DISABLED) {</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; SpeculateCellOperand op1(this, node-&gt;child1());</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; JITCompiler::Jump isOK = m_jit.branchStructurePtr(</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; JITCompiler::Equal,&nbsp;</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; JITCompiler::Address(op1.gpr(), JSCell::structureIDOffset()),&nbsp;</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; node-&gt;structure());</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; m_jit.breakpoint();</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; isOK.link(&amp;m_jit);</div><div>&nbsp; &nbsp; &nbsp; &nbsp; } else</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; speculateCell(node-&gt;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></body></html>