Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Thu, Dec 3, 2009 at 10:17 PM, Peter Kasting pkast...@google.com wrote: On Thu, Dec 3, 2009 at 6:24 PM, Chris Jerdonek chris.jerdo...@gmail.com wrote: For the people thinking about this, it would be nice if the final policy minimizes (and does not increase) the likelihood of having to modify several lines of surrounding code after touching one line of code. I don't think this consideration has been raised. Do you mean in the steady state of modifying the code base or while files aren't compliant with whatever change might get made? I meant the former (though my alternative below also reduces the latter). I think you mean the former, but if so, I'm not sure what policy would serve you best. Ideally you'd want always use braces, but failing that, each proposal has a different set of cases where you do/don't have to change as you touch things. I don't feel strongly about this, but I will provide an example to illustrate what I mean. The illustrative case is several else if clauses with braces -- only one of which exceeds one line. If a code change removes the excess lines in that one clause, the two rules being considered say you have to remove the braces from all the clauses -- even though the clauses are already lined up. And this change can go back and forth indefinitely. Several people are already saying they value alignment within individual if-else control structures more than they value the number of braces. So removing the braces from all the clauses in the example above seems secondary. An alternative policy is as follows: (1) If-else control structures must have either braces around all clauses or braces around no clauses. (2) A clause with more than one line must be surrounded by braces. A consequence of this policy is that if-else statements may gradually converge to braces, but this change would take place only as needed. --Chris ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Thursday 03 December 2009 02:30:17 pm Kenneth Russell wrote: In the WebKit WebGL implementation I've frequently encountered the case where the else clause in a single if/else is a one-liner, and I find it both ugly and error-prone to have to remove its braces. I'd really like to be able to use braces on both arms. Perhaps existing code could be grandfathered in but new or modified code required to conform to the rule Peter proposes. -Ken I'd like to lodge an objection on the grounds that the style guide is a matter of subjective opinion and rationally only serves to make our code consistent. I do not like changing this style guide as the fashion changes. However, as Hyatt might note I do not have a personally substantive problem with this particular change as I'd actually prefer this if the style guide were being made up today. It seems we keep changing the style guide as fashion changes. It is meant to ensure consistency and readability. This is a purely subjective change and I think unwarranted. Cheers, Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Fri, Dec 4, 2009 at 6:30 AM, Adam Treat tr...@kde.org wrote: On Thursday 03 December 2009 02:30:17 pm Kenneth Russell wrote: In the WebKit WebGL implementation I've frequently encountered the case where the else clause in a single if/else is a one-liner, and I find it both ugly and error-prone to have to remove its braces. I'd really like to be able to use braces on both arms. Perhaps existing code could be grandfathered in but new or modified code required to conform to the rule Peter proposes. -Ken I'd like to lodge an objection on the grounds that the style guide is a matter of subjective opinion and rationally only serves to make our code consistent. I do not like changing this style guide as the fashion changes. However, as Hyatt might note I do not have a personally substantive problem with this particular change as I'd actually prefer this if the style guide were being made up today. It seems we keep changing the style guide as fashion changes. It is meant to ensure consistency and readability. This is a purely subjective change and I think unwarranted. I'm not necessarily disagreeing with your conclusion, but I have to ask: can you think of an example of a style change that isn't subjective and/or something that can change as the fashion (or, more likely, the developers working on the project) changes? Even stuff like the namespace change really depends on your development style (to determine whether or not you care about those 4 extra spaces wasted). ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Friday 04 December 2009 12:17:03 pm Jeremy Orlow wrote: I'm not necessarily disagreeing with your conclusion, but I have to ask: can you think of an example of a style change that isn't subjective and/or something that can change as the fashion (or, more likely, the developers working on the project) changes? Even stuff like the namespace change really depends on your development style (to determine whether or not you care about those 4 extra spaces wasted). Very little of it is not subjective. The main thing the style guide gives us is consistency. And that I am fully supportive of. But a lot of it is merely taste. The compiler excepts both. Two developers fully informed and both of great technical prowess can reasonably disagree on almost all points of the style guide. That meets my definition of subjective for this purpose. Regarding the namespace change, I objected to that change too (and for the very same reasons) if you look through the archives. I don't think we should be changing the style guide for anything besides clarifications of currently unwritten rules. No matter how the fashion may change or how developers may change. Changing the rules throws consistency out the window which is, I believe, the greatest benefit of having a style guide. Cheers, Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Fri, Dec 4, 2009 at 9:39 AM, Adam Treat tr...@kde.org wrote: On Friday 04 December 2009 12:17:03 pm Jeremy Orlow wrote: I'm not necessarily disagreeing with your conclusion, but I have to ask: can you think of an example of a style change that isn't subjective and/or something that can change as the fashion (or, more likely, the developers working on the project) changes? Even stuff like the namespace change really depends on your development style (to determine whether or not you care about those 4 extra spaces wasted). Very little of it is not subjective. The main thing the style guide gives us is consistency. And that I am fully supportive of. But a lot of it is merely taste. The compiler excepts both. Two developers fully informed and both of great technical prowess can reasonably disagree on almost all points of the style guide. That meets my definition of subjective for this purpose. Regarding the namespace change, I objected to that change too (and for the very same reasons) if you look through the archives. I don't think we should be changing the style guide for anything besides clarifications of currently unwritten rules. No matter how the fashion may change or how developers may change. Changing the rules throws consistency out the window which is, I believe, the greatest benefit of having a style guide. Cheers, Adam You make a very compelling argument. Consistency first. I recant my support for the proposed change. -Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Fri, Dec 4, 2009 at 9:39 AM, Adam Treat tr...@kde.org wrote: I don't think we should be changing the style guide for anything besides clarifications of currently unwritten rules. No matter how the fashion may change or how developers may change. Changing the rules throws consistency out the window which is, I believe, the greatest benefit of having a style guide. While I agree with your points re: subjectivity, and I agree that any two competent programmers can disagree on any points, it is also true that some practices can be shown to be more reliable, maintainable, or readable, and those practices do change over time, partially as technology changes and partially because this is a social process. Hence I believe that if there was a significant consensus that rules should be changed, that is okay, even if there is no semantic difference in the rule change. I agree that having style guidelines that are not followed *in the existing codebase* are pretty much useless, even if they're used for new code. But, changing the rules only throws consistency out the window if the code is not updated to conform with the rule, so an alternative to an inconsistently-formatted codebase (and a useless style guide) or style-guide stais is to require whitespace-only CLs to update the codebase (and yes, I know some people object to such things). Accordingly, I believe that whitespace CLs are an acceptable cost to impose as a part of this, and people proposing the change should be willing to pay the cost. Of course, I think we should attempt to determine the cost before deciding to make the change. Dunno if this changes any of your thinking or not ... Peter, were you thinking that you (or at least someone) would attempt to bring the code into compliance with the new rule? -- Dirk ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Friday 04 December 2009 04:22:57 pm Dirk Pranke wrote: On Fri, Dec 4, 2009 at 9:39 AM, Adam Treat tr...@kde.org wrote: I don't think we should be changing the style guide for anything besides clarifications of currently unwritten rules. No matter how the fashion may change or how developers may change. Changing the rules throws consistency out the window which is, I believe, the greatest benefit of having a style guide. While I agree with your points re: subjectivity, and I agree that any two competent programmers can disagree on any points, it is also true that some practices can be shown to be more reliable, maintainable, or readable, and those practices do change over time, partially as technology changes and partially because this is a social process. You can't show that any practice is more readable than another because it is subjective as you admit. People can argue over the readability of different style options and come to different conclusions much as has been done in this long thread. As for technology changing, what do you think drives this particular style change other than the subjective opinions of a set of developers? Dunno if this changes any of your thinking or not ... Sorry, not really. Cheers, Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Fri, Dec 4, 2009 at 12:13 AM, Chris Jerdonek chris.jerdo...@gmail.comwrote: An alternative policy is as follows: (1) If-else control structures must have either braces around all clauses or braces around no clauses. (2) A clause with more than one line must be surrounded by braces. This is actually the precise policy used by the Google C++ Style Guide, which governs the Chromium repository. The problem with this is that it fails to ensure consistency, and the difference between consistently using braces everywhere and consistently not using them on one-line bodies is significant enough that this comes up as a bone of contention in code changes. On Fri, Dec 4, 2009 at 6:30 AM, Adam Treat tr...@kde.org wrote: It seems we keep changing the style guide as fashion changes. It is meant to ensure consistency and readability. This is a purely subjective change and I think unwarranted. Obviously if I agreed I wouldn't have proposed this change, but it's worth detailing why I disagree. You mention that the style guide is meant to ensure readability. It seems clear, then, that you would support changes to improve readability if they did not decrease consistency; the main problems are that readability is significantly subjective (see some of the expressed opinions in this thread), and changing the rules currently decreases consistency (at least over the short to medium term). I think safety and maintenance cost are also worth considering. My opinion is that this change (as well as Maciej's proposed variant) does not merely affect readability but also improves safety very slightly. (Requiring braces at all times would improve safety more, and decrease maintenance cost, but at perhaps some cost to readability.) I agree with you that our codebase should be consistent. Personally, I would prefer that when we make style guide changes, we actually change the codebase to comply with them. Most of the work here could probably be automated, and I think if we did this it would address the main concern you have. The counterargument to this is that it can obscure blame (probably moreso for a change like the namespace indenting one than this change, but noticeably in both cases). On the other hand, gradual changes obscure blame (gradually, at the changed locations) too, and it's already the case that when digging up the history of a block of code you need to repeatedly skip past uninteresting changes. On Fri, Dec 4, 2009 at 9:39 AM, Adam Treat tr...@kde.org wrote: I don't think we should be changing the style guide for anything besides clarifications of currently unwritten rules. No matter how the fashion may change or how developers may change. Changing the rules throws consistency out the window which is, I believe, the greatest benefit of having a style guide. Again, I think the consistency argument would be addressed if we actually made code consistent at rule changes. Changing code to make it more readable by the current developer set, when that developer set changes in makeup or opinion, seems like a good thing to me, as it improves productivity and code quality, regardless of whether the change is subjective in nature. As an example I give you the SQLite codebase, which has a consistent set of style rules that are so different from any other code I have ever encountered that I struggle to read and patch the code (and have been told the same by SQLite developers here at Google). You may argue that such a case is far outside any cognitive burden of the rule in question in this thread, but tiny amounts of cognitive friction, spread over a large codebase with many developers and a long lifetime, add up. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Fri, Dec 4, 2009 at 1:52 PM, Adam Treat tr...@kde.org wrote:
On Friday 04 December 2009 04:22:57 pm Dirk Pranke wrote:
On Fri, Dec 4, 2009 at 9:39 AM, Adam Treat tr...@kde.org wrote:
I don't think we should be changing the style guide for anything besides
clarifications of currently unwritten rules. No matter how the fashion
may change or how developers may change. Changing the rules throws
consistency out the window which is, I believe, the greatest benefit of
having a style guide.
While I agree with your points re: subjectivity, and I agree that any
two competent programmers can disagree on any
points, it is also true that some practices can be shown to be more
reliable, maintainable, or readable, and those
practices do change over time, partially as technology changes and
partially because this is a social process.
You can't show that any practice is more readable than another because it is
subjective as you admit. People can argue over the readability of different
style options and come to different conclusions much as has been done in this
long thread.
Not necessarily so; it depends on your definitions. If you define
more readable as more readable to me, then
you are correct. If you define it as a majority of the population
finds it more readable, then you can show this,
by a simple poll. I was using the latter definition. In addition,
people change their minds over time; I used to prefer:
if (x)
{
foo():
}
over KR, but I have gradually switched sides over the years ...
As for technology changing, what do you think drives this particular style
change other than the subjective opinions of a set of developers?
I don't believe this particular style change is being driven by a
technology change. I was
making a general counterargument to your general argument.
Dunno if this changes any of your thinking or not ...
Sorry, not really.
No worries :)
-- Dirk
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Dec 2, 2009, at 9:57 PM, Dan Bernstein wrote: I am satisfied with the existing rule. +1 ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Wed, Dec 2, 2009 at 9:57 PM, Dan Bernstein m...@apple.com wrote: On Dec 2, 2009, at 9:46 PM, Peter Kasting wrote: On Wed, Dec 2, 2009 at 9:19 PM, Mark Rowe mr...@apple.com wrote: On 2009-12-02, at 21:00, Peter Kasting wrote: I find this tricky to read and error-prone. I propose that the rule be modified to be: * When all arms of a conditional or loop are one physical line, do not use braces. If any arms are more than one physical line (even if they are one logical line), use braces on all arms. I do not agree that this would be an improvement. Are you satisfied with the existing rule, then? If so, you would be the first developer I have asked who is. I am satisfied with the existing rule. Me too. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
Nice, that change would make it compatible with the Qt style guide! I:-) I
say go for it!
Kenneth
On Thu, Dec 3, 2009 at 2:00 AM, Peter Kasting pkast...@google.com wrote:
This is a followup to my thread yesterday regarding consistent enforcement
of the style guide. Like Yong Li, I find the current rule about braces on
conditional arms to be suboptimal. The current rule is that one-line arms
must not have braces. This leads to strange constructions like:
if (foo) {
a;
b;
c;
// etc., very long body
} else
x;
...or perhaps:
if (foo)
a;
else if (bar) {
b;
c;
} else if (baz)
d;
else if (qux) {
e;
f;
}
I find this tricky to read and error-prone. I propose that the rule be
modified to be:
* When all arms of a conditional or loop are one physical line, do not use
braces. If any arms are more than one physical line (even if they are one
logical line), use braces on all arms.
In most places this will not differ from the existing code, so it will not
cause the whole codebase to become invalid; but it prevents cases where
the inconsistency leads (IMO) to lower readability/safety. (As a bonus for
Chromium developers, it's compatible with the Google style guide too,
although it goes further than that guide in order to make the correct style
explicit in all cases.)
PK
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
--
Kenneth Rohde Christiansen
Technical Lead / Senior Software Engineer
Qt Labs Americas, Nokia Technology Institute, INdT
Phone +55 81 8895 6002 / E-mail kenneth.christiansen at openbossa.org
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On 02.12.2009, at 23:35, Maciej Stachowiak wrote: One possible conservative modification to the rule is that if you have a multi-block if ... else if ... else chain, then if even one body has braces, all should. I think that would tend to reduce rather than increase visual noise, as all the else keywords would line up where right now they look ragged. That would be my preference, too. - WBR, Alexey Proskuryakov ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Wed, Dec 2, 2009 at 11:35 PM, Maciej Stachowiak m...@apple.com wrote:
I used to dislike everything about this rule; my original preference before
we codified our style guidelines was to always put braces around the body of
a control statement, even if it is only one line. Over time, I've gotten
used to it and I find this:
if (condA)
doA();
more pleasant to read than this:
if (condA) {
doA();
}
Even for if/else statements where only one branch is single-line, I'm ok
with the official style rule, although I can't say I actively like it. So
these two examples look ok to me:
if (condA)
doA();
else {
doB1();
doB2();
}
if (condA) {
doA1();
doA2();
} else
doB();
However, when there is a lengthy chain of if ... else if ... else
conditionals and a few of the blocks in the middle happens to be only a
single line each, then I tend to find the code harder to write, read and
modify. This pattern often comes up in the parseMappedAttribute()
implementations of Element subclasses.
Regardless of the specific outcome, I would strongly prefer to have a
consistent rule for this than to make it a matter of taste. One possible
conservative modification to the rule is that if you have a multi-block if
... else if ... else chain, then if even one body has braces, all should. I
think that would tend to reduce rather than increase visual noise, as all
the else keywords would line up where right now they look ragged.
I would also not object to bigger changes in the rule if that were truly the
community consensus, but personally I would set the barrier pretty high for
a rule change that would implicate large chunks of existing code, and I
think the benefit is much lower for if/else statements with only two
clauses.
In the WebKit WebGL implementation I've frequently encountered the
case where the else clause in a single if/else is a one-liner, and I
find it both ugly and error-prone to have to remove its braces. I'd
really like to be able to use braces on both arms.
Perhaps existing code could be grandfathered in but new or modified
code required to conform to the rule Peter proposes.
-Ken
Regards,
Maciej
On Dec 2, 2009, at 9:00 PM, Peter Kasting wrote:
This is a followup to my thread yesterday regarding consistent enforcement
of the style guide. Like Yong Li, I find the current rule about braces on
conditional arms to be suboptimal. The current rule is that one-line arms
must not have braces. This leads to strange constructions like:
if (foo) {
a;
b;
c;
// etc., very long body
} else
x;
...or perhaps:
if (foo)
a;
else if (bar) {
b;
c;
} else if (baz)
d;
else if (qux) {
e;
f;
}
I find this tricky to read and error-prone. I propose that the rule be
modified to be:
* When all arms of a conditional or loop are one physical line, do not use
braces. If any arms are more than one physical line (even if they are one
logical line), use braces on all arms.
In most places this will not differ from the existing code, so it will not
cause the whole codebase to become invalid; but it prevents cases where
the inconsistency leads (IMO) to lower readability/safety. (As a bonus for
Chromium developers, it's compatible with the Google style guide too,
although it goes further than that guide in order to make the correct style
explicit in all cases.)
PK
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Thu, Dec 3, 2009 at 11:20 AM, Alexey Proskuryakov a...@webkit.org wrote: On 02.12.2009, at 23:35, Maciej Stachowiak wrote: One possible conservative modification to the rule is that if you have a multi-block if ... else if ... else chain, then if even one body has braces, all should. I think that would tend to reduce rather than increase visual noise, as all the else keywords would line up where right now they look ragged. That would be my preference, too. I would like this. On Thu, Dec 3, 2009 at 11:30 AM, Kenneth Russell k...@google.com wrote: Perhaps existing code could be grandfathered in but new or modified code required to conform to the rule Peter proposes. This is more or less the norm. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Thu, Dec 3, 2009 at 11:36 AM, Jeremy Orlow jor...@chromium.org wrote: On Thu, Dec 3, 2009 at 11:20 AM, Alexey Proskuryakov a...@webkit.org wrote: On 02.12.2009, at 23:35, Maciej Stachowiak wrote: One possible conservative modification to the rule is that if you have a multi-block if ... else if ... else chain, then if even one body has braces, all should. I think that would tend to reduce rather than increase visual noise, as all the else keywords would line up where right now they look ragged. That would be my preference, too. I would like this. While I personally believe every branch should be wrapped in braces, it seems like we're not going to get consensus on that. I can live with no braces on any branch, but mixing brace-wrapped branches and non-wrapped is really distasteful. So, I would vote for this suggestion, as it gets us closer to what I believe is good. -- Dirk On Thu, Dec 3, 2009 at 11:30 AM, Kenneth Russell k...@google.com wrote: Perhaps existing code could be grandfathered in but new or modified code required to conform to the rule Peter proposes. This is more or less the norm. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Thu, Dec 3, 2009 at 11:36 AM, Jeremy Orlow jor...@chromium.org wrote: On Thu, Dec 3, 2009 at 11:20 AM, Alexey Proskuryakov a...@webkit.orgwrote: On 02.12.2009, at 23:35, Maciej Stachowiak wrote: One possible conservative modification to the rule is that if you have a multi-block if ... else if ... else chain, then if even one body has braces, all should. I think that would tend to reduce rather than increase visual noise, as all the else keywords would line up where right now they look ragged. That would be my preference, too. I would like this. +1 ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
While I personally believe every branch should be wrapped in braces, it seems like we're not going to get consensus on that. I can live with no braces on any branch, but mixing brace-wrapped branches and non-wrapped is really distasteful. So, I would vote for this suggestion, as it gets us closer to what I believe is good. +1 ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
what peter pitched and what mjs pitched are one and the same i think * When all arms of a conditional or loop are one physical line, do not use braces. If any arms are more than one physical line (even if they are one logical line), use braces on all arms. One possible conservative modification to the rule is that if you have a multi-block if ... else if ... else chain, then if even one body has braces, all should. I think that would tend to reduce rather than increase visual noise, as all the else keywords would line up where right now they look ragged. +1 ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Thu, Dec 3, 2009 at 5:48 PM, Michael Nordman micha...@google.com wrote: what peter pitched and what mjs pitched are one and the same i think No, they are not. Both of us would transform a conditional with three arms. Only I would transform one with two arms. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On 2009-12-03, at 17:48, Michael Nordman wrote: what peter pitched and what mjs pitched are one and the same i think They are different. Maciej's proposal was more conservative in that it only applies when multiple else blocks are present. * When all arms of a conditional or loop are one physical line, do not use braces. If any arms are more than one physical line (even if they are one logical line), use braces on all arms. One possible conservative modification to the rule is that if you have a multi-block if ... else if ... else chain, then if even one body has braces, all should. I think that would tend to reduce rather than increase visual noise, as all the else keywords would line up where right now they look ragged. +1 / πr2 - Mark smime.p7s Description: S/MIME cryptographic signature ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Dec 3, 2009, at 5:48 PM, Michael Nordman wrote: what peter pitched and what mjs pitched are one and the same i think * When all arms of a conditional or loop are one physical line, do not use braces. If any arms are more than one physical line (even if they are one logical line), use braces on all arms. One possible conservative modification to the rule is that if you have a multi-block if ... else if ... else chain, then if even one body has braces, all should. I think that would tend to reduce rather than increase visual noise, as all the else keywords would line up where right now they look ragged. To be clear: my intent was to propose something different than Peter's suggestion. Specifically, I suggest we do *not* change the rule for a simple if/else statement with only two clauses. Those would still be able to have braces on only one of the two clauses. But if/else chains of more than two clauses would have to be consistent. For anyone who agreed with my suggestion, you may want to consider whether you still agree in light of this clarification. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
Date: Wed, 2 Dec 2009 21:00:59 -0800 From: Peter Kasting pkast...@google.com This is a followup to my thread yesterday regarding consistent enforcement of the style guide. Like Yong Li, I find the current rule about braces on conditional arms to be suboptimal. For the people thinking about this, it would be nice if the final policy minimizes (and does not increase) the likelihood of having to modify several lines of surrounding code after touching one line of code. I don't think this consideration has been raised. This is similar to a point Darin Adler raised a few months back with regard to lining up comments: Things manually lined up in source code generally create a small maintainability problem. You can’t rename things without re-lining things up, and if you make other types code changes without noticing the lined-up comments the code ends up looking messy an peculiar. ( https://lists.webkit.org/pipermail/webkit-dev/2009-September/009814.html ) --Chris ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
I'm OK with both. Peter's is my preferred solution, though.
Kenneth
On Thu, Dec 3, 2009 at 11:39 PM, Dirk Pranke dpra...@chromium.org wrote:
+1. Peter's is better than Maciej's is better than status quo.
-- Dirk
On Thu, Dec 3, 2009 at 6:24 PM, Jeremy Orlow jor...@chromium.org wrote:
On Thu, Dec 3, 2009 at 6:15 PM, Maciej Stachowiak m...@apple.com wrote:
On Dec 3, 2009, at 5:48 PM, Michael Nordman wrote:
what peter pitched and what mjs pitched are one and the same i think
* When all arms of a conditional or loop are one physical line, do not
use
braces. If any arms are more than one physical line (even if they are
one
logical line), use braces on all arms.
One possible conservative modification to the rule is that if you
have
a multi-block if ... else if ... else chain, then if even one body
has
braces, all should. I think that would tend to reduce rather than
increase
visual noise, as all the else keywords would line up where right
now they
look ragged.
To be clear: my intent was to propose something different than Peter's
suggestion. Specifically, I suggest we do *not* change the rule for a
simple
if/else statement with only two clauses. Those would still be able to
have
braces on only one of the two clauses. But if/else chains of more than
two
clauses would have to be consistent.
For anyone who agreed with my suggestion, you may want to consider
whether
you still agree in light of this clarification.
Is this formally up for a vote then?
For what it's worth, I think Peters is the most readable, but Maciej's is
better than what it is now. I find stuff like
if (blah)
blah
else {
blah
}
very distracting. But stuff like
if (blah)
blah
else if (blah)
blah
else if (blah) {
blah
} else
blah
is definitely even worse.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
--
Kenneth Rohde Christiansen
Technical Lead / Senior Software Engineer
Qt Labs Americas, Nokia Technology Institute, INdT
Phone +55 81 8895 6002 / E-mail kenneth.christiansen at openbossa.org
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
Ditto.
On Thu, Dec 3, 2009 at 7:25 PM, Kenneth Christiansen
kenneth.christian...@openbossa.org wrote:
I'm OK with both. Peter's is my preferred solution, though.
Kenneth
On Thu, Dec 3, 2009 at 11:39 PM, Dirk Pranke dpra...@chromium.org wrote:
+1. Peter's is better than Maciej's is better than status quo.
-- Dirk
On Thu, Dec 3, 2009 at 6:24 PM, Jeremy Orlow jor...@chromium.org wrote:
On Thu, Dec 3, 2009 at 6:15 PM, Maciej Stachowiak m...@apple.com
wrote:
On Dec 3, 2009, at 5:48 PM, Michael Nordman wrote:
what peter pitched and what mjs pitched are one and the same i think
* When all arms of a conditional or loop are one physical line, do not
use
braces. If any arms are more than one physical line (even if they are
one
logical line), use braces on all arms.
One possible conservative modification to the rule is that if you
have
a multi-block if ... else if ... else chain, then if even one body
has
braces, all should. I think that would tend to reduce rather than
increase
visual noise, as all the else keywords would line up where right
now they
look ragged.
To be clear: my intent was to propose something different than Peter's
suggestion. Specifically, I suggest we do *not* change the rule for a
simple
if/else statement with only two clauses. Those would still be able to
have
braces on only one of the two clauses. But if/else chains of more than
two
clauses would have to be consistent.
For anyone who agreed with my suggestion, you may want to consider
whether
you still agree in light of this clarification.
Is this formally up for a vote then?
For what it's worth, I think Peters is the most readable, but Maciej's
is
better than what it is now. I find stuff like
if (blah)
blah
else {
blah
}
very distracting. But stuff like
if (blah)
blah
else if (blah)
blah
else if (blah) {
blah
} else
blah
is definitely even worse.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
--
Kenneth Rohde Christiansen
Technical Lead / Senior Software Engineer
Qt Labs Americas, Nokia Technology Institute, INdT
Phone +55 81 8895 6002 / E-mail kenneth.christiansen at openbossa.org
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] Proposing style guide change regarding braces on conditional arms
This is a followup to my thread yesterday regarding consistent enforcement
of the style guide. Like Yong Li, I find the current rule about braces on
conditional arms to be suboptimal. The current rule is that one-line arms
must not have braces. This leads to strange constructions like:
if (foo) {
a;
b;
c;
// etc., very long body
} else
x;
...or perhaps:
if (foo)
a;
else if (bar) {
b;
c;
} else if (baz)
d;
else if (qux) {
e;
f;
}
I find this tricky to read and error-prone. I propose that the rule be
modified to be:
* When all arms of a conditional or loop are one physical line, do not use
braces. If any arms are more than one physical line (even if they are one
logical line), use braces on all arms.
In most places this will not differ from the existing code, so it will not
cause the whole codebase to become invalid; but it prevents cases where
the inconsistency leads (IMO) to lower readability/safety. (As a bonus for
Chromium developers, it's compatible with the Google style guide too,
although it goes further than that guide in order to make the correct style
explicit in all cases.)
PK
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On 2009-12-02, at 21:00, Peter Kasting wrote: I find this tricky to read and error-prone. I propose that the rule be modified to be: * When all arms of a conditional or loop are one physical line, do not use braces. If any arms are more than one physical line (even if they are one logical line), use braces on all arms. I do not agree that this would be an improvement. In most places this will not differ from the existing code, so it will not cause the whole codebase to become invalid; I glanced briefly at three files: RenderObject.cpp, Element.cpp and Node.cpp. In these three files I counted over two dozen places that would require modification to conform with this new rule. That's by no means a majority of the relevant statements in these files, but it's not a small number either. - Mark smime.p7s Description: S/MIME cryptographic signature ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Wed, Dec 2, 2009 at 9:19 PM, Mark Rowe mr...@apple.com wrote: On 2009-12-02, at 21:00, Peter Kasting wrote: I find this tricky to read and error-prone. I propose that the rule be modified to be: * When all arms of a conditional or loop are one physical line, do not use braces. If any arms are more than one physical line (even if they are one logical line), use braces on all arms. I do not agree that this would be an improvement. Are you satisfied with the existing rule, then? If so, you would be the first developer I have asked who is. Admittedly my sample size is quite small, but all those I've asked would prefer either a rule like my proposal or a rule that says braces always, which I find to be consistent and easy to remember but somewhat verbose (and with a greater impact on the existing code, for what that's worth). In most places this will not differ from the existing code, so it will not cause the whole codebase to become invalid; I glanced briefly at three files: RenderObject.cpp, Element.cpp and Node.cpp. In these three files I counted over two dozen places that would require modification to conform with this new rule. That's by no means a majority of the relevant statements in these files, but it's not a small number either. I stand by my statement. In Element.cpp, 9 conditionals violate this out of roughly 200 conditionals and loops. One of these violates the existing style guide too. Not a majority is a significant understatement. Especially when compared to our namespace change that affects nearly every header, or the proposed case indenting change that affects every switch statement. That said, I don't think doesn't change a large percentage of the code is a significant argument for a rule, it mostly speaks to how people wouldn't need to massively shift their thinking when writing code because the rules have changed. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Dec 2, 2009, at 9:46 PM, Peter Kasting wrote: On Wed, Dec 2, 2009 at 9:19 PM, Mark Rowe mr...@apple.com wrote: On 2009-12-02, at 21:00, Peter Kasting wrote: I find this tricky to read and error-prone. I propose that the rule be modified to be: * When all arms of a conditional or loop are one physical line, do not use braces. If any arms are more than one physical line (even if they are one logical line), use braces on all arms. I do not agree that this would be an improvement. Are you satisfied with the existing rule, then? If so, you would be the first developer I have asked who is. I am satisfied with the existing rule.___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On 2009-12-02, at 21:46, Peter Kasting wrote: On Wed, Dec 2, 2009 at 9:19 PM, Mark Rowe mr...@apple.com wrote: On 2009-12-02, at 21:00, Peter Kasting wrote: I find this tricky to read and error-prone. I propose that the rule be modified to be: * When all arms of a conditional or loop are one physical line, do not use braces. If any arms are more than one physical line (even if they are one logical line), use braces on all arms. I do not agree that this would be an improvement. Are you satisfied with the existing rule, then? I am satisfied with the existing rule, and I have seen nothing to support the suggestion that a change to the rule would provide additional benefits. In most places this will not differ from the existing code, so it will not cause the whole codebase to become invalid; I glanced briefly at three files: RenderObject.cpp, Element.cpp and Node.cpp. In these three files I counted over two dozen places that would require modification to conform with this new rule. That's by no means a majority of the relevant statements in these files, but it's not a small number either. I stand by my statement. I was not putting this forward as my reasoning for not changing the style, merely as a data point. - Mark smime.p7s Description: S/MIME cryptographic signature ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
Are you satisfied with the existing rule, then? If so, you would be the first developer I have asked who is. I am satisfied with the existing rule. I'm frustrated by this existing rule. I would be very happy if the rule was changed to We may omit braces for one-line control clauses. -- TAMURA Kent Software Engineer, Google ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
I used to dislike everything about this rule; my original preference
before we codified our style guidelines was to always put braces
around the body of a control statement, even if it is only one line.
Over time, I've gotten used to it and I find this:
if (condA)
doA();
more pleasant to read than this:
if (condA) {
doA();
}
Even for if/else statements where only one branch is single-line, I'm
ok with the official style rule, although I can't say I actively like
it. So these two examples look ok to me:
if (condA)
doA();
else {
doB1();
doB2();
}
if (condA) {
doA1();
doA2();
} else
doB();
However, when there is a lengthy chain of if ... else if ... else
conditionals and a few of the blocks in the middle happens to be only
a single line each, then I tend to find the code harder to write, read
and modify. This pattern often comes up in the parseMappedAttribute()
implementations of Element subclasses.
Regardless of the specific outcome, I would strongly prefer to have a
consistent rule for this than to make it a matter of taste. One
possible conservative modification to the rule is that if you have a
multi-block if ... else if ... else chain, then if even one body has
braces, all should. I think that would tend to reduce rather than
increase visual noise, as all the else keywords would line up where
right now they look ragged.
I would also not object to bigger changes in the rule if that were
truly the community consensus, but personally I would set the barrier
pretty high for a rule change that would implicate large chunks of
existing code, and I think the benefit is much lower for if/else
statements with only two clauses.
Regards,
Maciej
On Dec 2, 2009, at 9:00 PM, Peter Kasting wrote:
This is a followup to my thread yesterday regarding consistent
enforcement of the style guide. Like Yong Li, I find the current
rule about braces on conditional arms to be suboptimal. The current
rule is that one-line arms must not have braces. This leads to
strange constructions like:
if (foo) {
a;
b;
c;
// etc., very long body
} else
x;
...or perhaps:
if (foo)
a;
else if (bar) {
b;
c;
} else if (baz)
d;
else if (qux) {
e;
f;
}
I find this tricky to read and error-prone. I propose that the rule
be modified to be:
* When all arms of a conditional or loop are one physical line, do
not use braces. If any arms are more than one physical line (even
if they are one logical line), use braces on all arms.
In most places this will not differ from the existing code, so it
will not cause the whole codebase to become invalid; but it
prevents cases where the inconsistency leads (IMO) to lower
readability/safety. (As a bonus for Chromium developers, it's
compatible with the Google style guide too, although it goes further
than that guide in order to make the correct style explicit in all
cases.)
PK
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
