Re: [webkit-dev] Style proposal for branch style for macros
On Apr 20, 2014, at 1:32 PM, Mark Rowe wrote:
>
> 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
> template struct ValueCheck> {
> typedef Vector TraitType;
> static void checkConsistency(const Vector& v)
> {
> v.checkConsistency();
> }
> };
> #endif
>
> template
> inline void Vector::checkConsistency()
> {
> if (!ASSERT_DISABLED) {
> for (size_t i = 0; i < size(); ++i)
> ValueCheck::checkConsistency(at(i));
> }
> }
>
>
> This won’t compile with assertions disabled because
> ValueCheck::checkConsistency doesn’t exist.
Oops, I should have read this email before sending my previous reply. It does
seem like using if() exclusively to control assertions won’t work.
On Apr 20, 2014, at 2:02 PM, Filip Pizlo wrote:
> The #if means that the code being guarded isn't indented. I believe that
> indenting control flow is a good idea.
This is the main reason I thought the proposal was promising. Note though that
we could change our style guide to call for some indentation of #if’s if we
wanted to. It is valid C/C++ to do so. The main is that not all text editors
have good support for this.
Regards,
Maciej
___
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 wrote:
>
>
> Won’t compile:
>
> if (!MAYBE_DEFINED) {
> …
> }
>
> if (SOMETHING) {
> void doSomething() { }
> }
>
> if (SOMETHING) {
> #include “Something.h"
> }
>
>
> Will result in unintended behavior:
>
> if (!FOO) {
> …
> #define BAR
> …
> }
We clearly can’t use if() to replace #if in general. Let’s assume that we
reject a universal proposal of that sort.
However, it seems like the above examples will generally not apply in the case
of the switch for assertions being enabled or disabled. Or at least I would not
expect it - do you know of such cases in the code?
One worry about Phil’s proposal is that we’d be making asserts enabled/disabled
different from handling of other conditional preprocessor macros. One possible
way to avoid this is to make asserts enabled/disabled a compile-time constant
held in a const global variable instead. That would enforce this style and make
it natural.
I am not sure offhand if this is actually viable, but I see no deep reason that
assertion control has to be just like the conditional macros we use to avoid
submitting code to the compiler that won’t actually compile.
Regards,
Maciej
___
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 wrote: > > >> On Apr 20, 2014, at 14:50, Filip Pizlo wrote: >> >> >> On Apr 20, 2014, at 2:43 PM, Mark Rowe wrote: On Apr 20, 2014, at 14:02, Filip Pizlo 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
Re: [webkit-dev] Style proposal for branch style for macros
On Apr 20, 2014, at 14:50, Filip Pizlo wrote: > > >> On Apr 20, 2014, at 2:43 PM, Mark Rowe wrote: >> >> >>> On Apr 20, 2014, at 14:02, Filip Pizlo 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 2:43 PM, Mark Rowe wrote: > > >> On Apr 20, 2014, at 14:02, Filip Pizlo 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:02, Filip Pizlo 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
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 wrote:
>
>
>> On Apr 19, 2014, at 13:36, Filip Pizlo 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 1:19 PM, Mark Rowe wrote:
>
>
>> On Apr 20, 2014, at 13:08, Filip Pizlo wrote:
>>
>>
>>
>>> On Apr 20, 2014, at 12:56 PM, Mark Rowe wrote:
>>>
>>>
On Apr 19, 2014, at 13:09, Filip Pizlo 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
On Apr 20, 2014, at 13:19, Mark Rowe wrote:
>
> On Apr 20, 2014, at 13:08, Filip Pizlo wrote:
>
>>
>>
>> On Apr 20, 2014, at 12:56 PM, Mark Rowe wrote:
>>
>>>
>>> On Apr 19, 2014, at 13:09, Filip Pizlo 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
template struct ValueCheck> {
typedef Vector TraitType;
static void checkConsistency(const Vector& v)
{
v.checkConsistency();
}
};
#endif
template
inline void Vector::checkConsistency()
{
if (!ASSERT_DISABLED) {
for (size_t i = 0; i < size(); ++i)
ValueCheck::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 19, 2014, at 13:36, Filip Pizlo 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:08, Filip Pizlo wrote:
>
>
> On Apr 20, 2014, at 12:56 PM, Mark Rowe wrote:
>
>>
>> On Apr 19, 2014, at 13:09, Filip Pizlo 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 20, 2014, at 12:56 PM, Mark Rowe wrote:
>
>
>> On Apr 19, 2014, at 13:09, Filip Pizlo 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 12:18 PM, Geoffrey Garen 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 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 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/declara
Re: [webkit-dev] Style proposal for branch style for macros
On Apr 19, 2014, at 13:09, Filip Pizlo 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
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 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 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
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 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
Can you give a motivating example from WebKit?
Geoff
On Apr 19, 2014, at 1:09 PM, Filip Pizlo 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 4/19/14, 1:09 PM, Filip Pizlo wrote:
> 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.
This is adding complexity for little gain.
I would not mind replacing all the macros by real if() branches, but
that is not possible in plenty of cases. Using both the branches and the
macro seems like a bad idea.
Benjamin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
[webkit-dev] Style proposal for branch style for macros
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
