Re: [webkit-dev] Style proposal for branch style for macros
Seems like an improvement to me.
Two other things that would improve this code for me:
(1) “Not doing no assertions” hurts my brain. I wish we could use “if
(ASSERT_ENABLED)” instead.
(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.
Geoff
On Apr 19, 2014, at 1:36 PM, 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.
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
Re: [webkit-dev] Style proposal for branch style for macros
On Apr 19, 2014, at 13:09, 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?
It’d be one thing if we could adopt this approach everywhere, but as you note
there are numerous situations in which it won’t compile. What’s worse is that
there are situations in which it’ll silently give unintended behavior. It’s not
clear to me what benefit you see this proposal bringing, and why it’d be worth
the complexity that it adds. Given the lack of a compelling argument in favor
of this change I’m firmly against it.
- Mark
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Style proposal for branch style for macros
On Apr 20, 2014, at 12:18 PM, Geoffrey Garen gga...@apple.com wrote:
Seems like an improvement to me.
Two other things that would improve this code for me:
(1) “Not doing no assertions” hurts my brain. I wish we could use “if
(ASSERT_ENABLED)” instead.
+1
(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.
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.
Geoff
On Apr 19, 2014, at 1:36 PM, 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.
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
Re: [webkit-dev] Style proposal for branch style for macros
On Apr 20, 2014, at 12:56 PM, Mark Rowe mr...@apple.com wrote:
On Apr 19, 2014, at 13:09, 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?
It’d be one thing if we could adopt this approach everywhere, but as you note
there are numerous situations in which it won’t compile. What’s worse is that
there are situations in which it’ll silently give unintended behavior.
Can you give an example of this?
It’s not clear to me what benefit you see this proposal bringing, and why
it’d be worth the complexity that it adds. Given the lack of a compelling
argument in favor of this change I’m firmly against it.
I would be firmly against changing any of the existing if (!ASSERT_DISABLED)
into #if's as this would make already-gnarly JIT code even harder to follow.
- Mark
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Style proposal for branch style for macros
On Apr 20, 2014, at 13:08, Filip Pizlo fpi...@apple.com wrote:
On Apr 20, 2014, at 12:56 PM, Mark Rowe mr...@apple.com wrote:
On Apr 19, 2014, at 13:09, 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?
It’d be one thing if we could adopt this approach everywhere, but as you
note there are numerous situations in which it won’t compile. What’s worse
is that there are situations in which it’ll silently give unintended
behavior.
Can you give an example of this?
Won’t compile:
if (!MAYBE_DEFINED) {
…
}
if (SOMETHING) {
void doSomething() { }
}
if (SOMETHING) {
#include “Something.h
}
Will result in unintended behavior:
if (!FOO) {
…
#define BAR
…
}
It’s not clear to me what benefit you see this proposal bringing, and why
it’d be worth the complexity that it adds. Given the lack of a compelling
argument in favor of this change I’m firmly against it.
I would be firmly against changing any of the existing if
(!ASSERT_DISABLED) into #if's as this would make already-gnarly JIT code
even harder to follow.
So you’re only interested in the wider community’s opinion on this style
proposal if they agree with you, and if not you’ll just ignore them and keep
doing your own thing?
- Mark
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Style proposal for branch style for macros
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
Re: [webkit-dev] Style proposal for branch style for macros
On Apr 20, 2014, at 13:19, Mark Rowe mr...@apple.com wrote:
On Apr 20, 2014, at 13:08, Filip Pizlo fpi...@apple.com wrote:
On Apr 20, 2014, at 12:56 PM, Mark Rowe mr...@apple.com wrote:
On Apr 19, 2014, at 13:09, 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?
It’d be one thing if we could adopt this approach everywhere, but as you
note there are numerous situations in which it won’t compile. What’s worse
is that there are situations in which it’ll silently give unintended
behavior.
Can you give an example of this?
Won’t compile:
if (!MAYBE_DEFINED) {
…
}
if (SOMETHING) {
void doSomething() { }
}
if (SOMETHING) {
#include “Something.h
}
It’s also not possible to determine from context alone whether #if or if should
be used. When #if is used to remove members or functions you’re then forced to
use #if, rather than if, around all uses of those members or functions since
the alternative won’t compile.
An example lifted from Vector.h:
#if !ASSERT_DISABLED
templatetypename T struct ValueCheckVectorT {
typedef VectorT TraitType;
static void checkConsistency(const VectorT v)
{
v.checkConsistency();
}
};
#endif
templatetypename T, size_t inlineCapacity, typename OverflowHandler
inline void VectorT, inlineCapacity, OverflowHandler::checkConsistency()
{
if (!ASSERT_DISABLED) {
for (size_t i = 0; i size(); ++i)
ValueCheckT::checkConsistency(at(i));
}
}
This won’t compile with assertions disabled because
ValueCheck::checkConsistency doesn’t exist.
- Mark
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Style proposal for branch style for macros
On Apr 20, 2014, at 1:19 PM, Mark Rowe mr...@apple.com wrote:
On Apr 20, 2014, at 13:08, Filip Pizlo fpi...@apple.com wrote:
On Apr 20, 2014, at 12:56 PM, Mark Rowe mr...@apple.com wrote:
On Apr 19, 2014, at 13:09, 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?
It’d be one thing if we could adopt this approach everywhere, but as you
note there are numerous situations in which it won’t compile. What’s worse
is that there are situations in which it’ll silently give unintended
behavior.
Can you give an example of this?
Won’t compile:
if (!MAYBE_DEFINED) {
…
}
if (SOMETHING) {
void doSomething() { }
}
if (SOMETHING) {
#include “Something.h
}
Will result in unintended behavior:
if (!FOO) {
…
#define BAR
…
}
If it won't even compile then I wouldn't worry about it.
As for conditionalized #define's and such, it's already rare and discouraged to
have those inside function definition bodies.
It’s not clear to me what benefit you see this proposal bringing, and why
it’d be worth the complexity that it adds. Given the lack of a compelling
argument in favor of this change I’m firmly against it.
I would be firmly against changing any of the existing if
(!ASSERT_DISABLED) into #if's as this would make already-gnarly JIT code
even harder to follow.
So you’re only interested in the wider community’s opinion on this style
proposal if they agree with you, and if not you’ll just ignore them and keep
doing your own thing?
No. The style currently allows the use of #if's inside function bodies even if
doing so makes the code ugly. It also allows the use of regular conditionals
when the predicate being tested happens to be a macro (as in if
(!ASSERT_DISABLED)).
- Mark
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Style proposal for branch style for macros
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
Re: [webkit-dev] Style proposal for branch style for macros
On Apr 20, 2014, at 14:02, Filip Pizlo fpi...@apple.com wrote: 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. We use preprocessor defines to distinguish between many things besides debug and release builds, and in many cases it’s impossible to tell ahead of time whether the alternate paths will compile on all platforms. This is particularly true for code guarded by defines related to the compiler, target platform, or OS version, but is also a concern with feature defines where symbols have been #if’d out. 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. I don’t think this is a great analogy. It’s usually clear from the context whether what you’re asserting is a compile-time or run-time concept, and thus whether a compile-time or run-time assertion is appropriate. It’s far from obvious whether all branches of an if will compile. - Mark ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Style proposal for branch style for macros
On Apr 20, 2014, at 2:43 PM, Mark Rowe mr...@apple.com wrote: On Apr 20, 2014, at 14:02, Filip Pizlo fpi...@apple.com wrote: 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. We use preprocessor defines to distinguish between many things besides debug and release builds, and in many cases it’s impossible to tell ahead of time whether the alternate paths will compile on all platforms. This is particularly true for code guarded by defines related to the compiler, target platform, or OS version, but is also a concern with feature defines where symbols have been #if’d out. 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. I don’t think this is a great analogy. It’s usually clear from the context whether what you’re asserting is a compile-time or run-time concept, and thus whether a compile-time or run-time assertion is appropriate. It’s far from obvious whether all branches of an if will compile. In the cases that I'm concerned with, I don't care if the code guarded by ASSERT_DISABLED will compile, only whether it will run. This is mostly, but not entirely, different from CPU guards - even those may be dynamic like in the case of tests for exotic SSE capabilities. I don't think you can make a hard and fast rule based on the compile versus run distinction. You can make one based on semantic equivalence (if it's equivalent then avoid the preprocessor). So, I think that if the only goal here was to make an enforceable rule then my proposal is somewhat better. - Mark ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Style proposal for branch style for macros
On Apr 20, 2014, at 14:50, Filip Pizlo fpi...@apple.com wrote: On Apr 20, 2014, at 2:43 PM, Mark Rowe mr...@apple.com wrote: On Apr 20, 2014, at 14:02, Filip Pizlo fpi...@apple.com wrote: 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. We use preprocessor defines to distinguish between many things besides debug and release builds, and in many cases it’s impossible to tell ahead of time whether the alternate paths will compile on all platforms. This is particularly true for code guarded by defines related to the compiler, target platform, or OS version, but is also a concern with feature defines where symbols have been #if’d out. 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. I don’t think this is a great analogy. It’s usually clear from the context whether what you’re asserting is a compile-time or run-time concept, and thus whether a compile-time or run-time assertion is appropriate. It’s far from obvious whether all branches of an if will compile. In the cases that I'm concerned with, I don't care if the code guarded by ASSERT_DISABLED will compile, only whether it will run. This is mostly, but not entirely, different from CPU guards - even those may be dynamic like in the case of tests for exotic SSE capabilities. I’m not sure what you’re trying to say here. Surely you care that the code compiles without warnings, since otherwise you’d break the build. Can you elaborate on the difference you see between ASSERT_DISABLED checks and platform-related checks (OS, OS version, CPU architecture, etc), and what impact that difference has on your proposal? I don't think you can make a hard and fast rule based on the compile versus run distinction. You can make one based on semantic equivalence (if it's equivalent then avoid the preprocessor). So, I think that if the only goal here was to make an enforceable rule then my proposal is somewhat better. I’m not sure exactly what you mean by semantic equivalence here. #if vs if inherently have different semantics. - Mark ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Style proposal for branch style for macros
On Apr 20, 2014, at 3:09 PM, Mark Rowe mr...@apple.com wrote: On Apr 20, 2014, at 14:50, Filip Pizlo fpi...@apple.com wrote: On Apr 20, 2014, at 2:43 PM, Mark Rowe mr...@apple.com wrote: On Apr 20, 2014, at 14:02, Filip Pizlo fpi...@apple.com wrote: 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. We use preprocessor defines to distinguish between many things besides debug and release builds, and in many cases it’s impossible to tell ahead of time whether the alternate paths will compile on all platforms. This is particularly true for code guarded by defines related to the compiler, target platform, or OS version, but is also a concern with feature defines where symbols have been #if’d out. 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. I don’t think this is a great analogy. It’s usually clear from the context whether what you’re asserting is a compile-time or run-time concept, and thus whether a compile-time or run-time assertion is appropriate. It’s far from obvious whether all branches of an if will compile. In the cases that I'm concerned with, I don't care if the code guarded by ASSERT_DISABLED will compile, only whether it will run. This is mostly, but not entirely, different from CPU guards - even those may be dynamic like in the case of tests for exotic SSE capabilities. I’m not sure what you’re trying to say here. Surely you care that the code compiles without warnings, since otherwise you’d break the build. I may have misunderstood your point and gone off on a tangent, so this may no longer be related to my proposal. What I mean by the code will compile is whether the compiler will see the code after preprocessing completes. That's the key distinction between #if and if - #if prevents code from being compiled. Can you elaborate on the difference you see between ASSERT_DISABLED checks and platform-related checks (OS, OS version, CPU architecture, etc), and what impact that difference has on your proposal? I don't think it impacts my proposal at all - if it was possible to case on CPU without using #if and it would result in the same run-time behavior we would have gotten from the #if, then I would prefer to do it would the preprocessor to keep the code cleaner. I don't think you can make a hard and fast rule based on the compile versus run distinction. You can make one based on semantic equivalence (if it's equivalent then avoid the preprocessor). So, I think that if the only goal here was to make an enforceable rule then my proposal is somewhat better. I’m not sure exactly what you mean by semantic equivalence here. #if vs if inherently have different semantics. I mean that if in a particular case, using #if and if would result in identical behavior, then if should be used. Obviously this doesn't apply to all cases because it is predicated on there being identical behavior and of course the behavior won't always be identical. - Mark ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
