<html><head><style>body{font-family:Helvetica,Arial;font-size:13px}</style></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div id="bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px; color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;">Here are some examples.</div><div id="bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px; color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;"><br></div><div id="bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px; color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;">Guarding a loop that does assertions:</div><div id="bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px; color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;"><br></div><div id="bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px; color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;"><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><br></div><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><br></div><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><br></div><div>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.</div><div><br></div></div> <div id="bloop_sign_1397938972036544000" class="bloop_sign"><div style="font-family:helvetica,arial;font-size:13px">-Phil</div><div style="font-family:helvetica,arial;font-size:13px"><br></div></div> <br><p style="color:#000;">On April 19, 2014 at 1:22:23 PM, Geoffrey Garen (<a href="mailto:ggaren@apple.com">ggaren@apple.com</a>) wrote:</p> <blockquote type="cite" class="clean_bq"><span><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div></div><div>
<title></title>
Can you give a motivating example from WebKit?
<div><br></div>
<div>Geoff</div>
<div><br>
<div>
<div>On Apr 19, 2014, at 1:09 PM, Filip Pizlo <<a href="mailto:fpizlo@apple.com">fpizlo@apple.com</a>> wrote:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite">
<div 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; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">
<div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; margin: 0px;">Hey
everyone,</div>
<div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; margin: 0px;">
<br></div>
<div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; margin: 0px;">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:</div>
<div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; margin: 0px;">
<br></div>
<div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; margin: 0px;">if
(!ASSERT_DISABLED) {</div>
<div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; margin: 0px;">
// do things</div>
<div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; margin: 0px;">
}</div>
<div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; margin: 0px;">
<br></div>
<div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; margin: 0px;">
and:</div>
<div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; margin: 0px;">
<br></div>
<div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; margin: 0px;">#if
!ASSERT_DISABLED</div>
<div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; margin: 0px;">//
do things</div>
<div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; margin: 0px;">
#endif</div>
<div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; margin: 0px;">
<br></div>
<div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; margin: 0px;">I'd
like to propose that<span class="Apple-converted-space"> </span><i>anytime the normal if would
be semantically equivalent to the preprocessor #if, the normal if
should be used</i>.</div>
<div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; margin: 0px;">
<br></div>
<div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; margin: 0px;">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.</div>
<div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; margin: 0px;">
<br></div>
<div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; margin: 0px;">The
"semantically equivalent" requirement still allows for #if to be
used for thinngs like:</div>
<div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; margin: 0px;">
<br></div>
<div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; margin: 0px;">-
Guarding the placement of fields in a class.</div>
<div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; margin: 0px;">-
Guarding the definitions of other macros (in the case of
ASSERT_DISABLED, we define ASSERT in different ways guarded by
#if's)</div>
<div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; margin: 0px;">-
Guarding the definition/declaration/inclusion of entire functions,
classes, and other top-level constructs.</div>
<div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; margin: 0px;">
<br></div>
<div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; margin: 0px;">
Thoughts?</div>
<br>
<div id="bloop_sign_1397937780250571008" class="bloop_sign">
<div style="font-family: helvetica, arial; font-size: 13px;">
-Phil</div>
<div style="font-family: helvetica, arial; font-size: 13px;">
<br></div>
</div>
_______________________________________________<br>
webkit-dev mailing list<br>
<a href="mailto:webkit-dev@lists.webkit.org">webkit-dev@lists.webkit.org</a><br>
<a href="https://lists.webkit.org/mailman/listinfo/webkit-dev">https://lists.webkit.org/mailman/listinfo/webkit-dev</a></div>
</blockquote>
</div>
<br></div>
</div></div></span></blockquote></body></html>