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.
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.
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());
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.
-Phil
On April 19, 2014 at 1:22:23 PM, Geoffrey Garen (gga...@apple.com) wrote:
Can you give a motivating example from WebKit?
Geoff
On Apr 19, 2014, at 1:09 PM, Filip Pizlo <fpi...@apple.com> wrote:
Hey everyone,
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:
if (!ASSERT_DISABLED) {
// do things
}
and:
#if !ASSERT_DISABLED
// do things
#endif
I'd like to propose that anytime the normal if would be semantically equivalent
to the preprocessor #if, the normal if should be used.
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.
The "semantically equivalent" requirement still allows for #if to be used for
thinngs like:
- Guarding the placement of fields in a class.
- Guarding the definitions of other macros (in the case of ASSERT_DISABLED, we
define ASSERT in different ways guarded by #if's)
- Guarding the definition/declaration/inclusion of entire functions, classes,
and other top-level constructs.
Thoughts?
-Phil
_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
