<html><head><meta http-equiv="content-type" content="text/html; charset=utf-8"></head><body dir="auto"><div><br></div><div><br>On Apr 20, 2014, at 12:18 PM, Geoffrey Garen <<a href="mailto:ggaren@apple.com">ggaren@apple.com</a>> wrote:<br><br></div><blockquote type="cite"><div><meta http-equiv="Content-Type" content="text/html charset=windows-1252">Seems like an improvement to me.<div><br></div><div>Two other things that would improve this code for me:</div><div><br></div><div>(1) “Not doing no assertions” hurts my brain. I wish we could use “if (ASSERT_ENABLED)” instead.</div></div></blockquote><div><br></div><div>+1</div><br><blockquote type="cite"><div><div><br></div><div>(2) ASSERT is easier to understand if it only adds code, and does not remove code. So, in the “ASSERT else speculateCellI()” code, I’d prefer to make the speculateCell() unconditional. That’s easier to read. It’s true that the speculateCell() is redundant with the previous ASSERT, but there’s no harm done by that.</div></div></blockquote><div><br></div><div>Unfortunately, the weirdness of the DFG's register allocator means that that doing a speculateCell and then a SpeculateCellOperand may cause issues. We expect an operant to be used at most once, and this would sort of look like two uses. </div><br><blockquote type="cite"><div><div><br></div><div>Geoff</div><div><br><div><div>On Apr 19, 2014, at 1:36 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;">Here are some examples.</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 a loop that does assertions:</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;"><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="">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>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></div></div></div></span></blockquote></div></blockquote></div><br></div></div></blockquote></body></html>