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 <mr...@apple.com> wrote:
>
>
>> On Apr 19, 2014, at 13:36, Filip Pizlo <fpi...@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
>
_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
