No, it's just a matter of coding style used in the kernel.
What is a logical basis for adopting this particular rule? Sometimes it is not just a matter of preference.
These parentheses are unnecessary for humans, and if they are not needed for compilers or some other tools, they are bad! Without them, the bug could be noticed earlier.
Cheers, Dmitry
Good, let's discuss it developers-wide then, and add a result into the (to become official) Coding Style Guidelines for kernel-mode code. The only thing I'm bothered with is to keep the style consistent across kernel (and drivers, where possible). And right now bracing arguments of && and || prevails.
I'm eager to listen to other dev's opinion on that topic. You just showed one disadvantage of such method. Are there any advantages?
WBR, Aleksey.
On Feb 16, 2009, at 8:47 PM, Dmitry Gorbachev wrote:
No, it's just a matter of coding style used in the kernel.
What is a logical basis for adopting this particular rule? Sometimes it is not just a matter of preference.
These parentheses are unnecessary for humans, and if they are not needed for compilers or some other tools, they are bad! Without them, the bug could be noticed earlier.
Cheers, Dmitry
IMO parenthesis should always be used, it helps to clarify what the developer who wrote the code really wanted.
I also find code which makes use of parenthesis much easier to read quickly, although that's probably just a personal preference.
Ged.
-----Original Message----- From: ros-dev-bounces@reactos.org [mailto:ros-dev-bounces@reactos.org] On Behalf Of Aleksey Bragin Sent: 16 February 2009 18:27 To: ReactOS Development List Subject: Re: [ros-dev] [ros-diffs] [dgorbachev] 39422: Fix bug #4129 in CmpGetNameControlBlock().
Good, let's discuss it developers-wide then, and add a result into the (to become official) Coding Style Guidelines for kernel-mode code. The only thing I'm bothered with is to keep the style consistent across kernel (and drivers, where possible). And right now bracing arguments of && and || prevails.
I'm eager to listen to other dev's opinion on that topic. You just showed one disadvantage of such method. Are there any advantages?
WBR, Aleksey.
On Feb 16, 2009, at 8:47 PM, Dmitry Gorbachev wrote:
No, it's just a matter of coding style used in the kernel.
What is a logical basis for adopting this particular rule? Sometimes it is not just a matter of preference.
These parentheses are unnecessary for humans, and if they are not needed for compilers or some other tools, they are bad! Without them, the bug could be noticed earlier.
Cheers, Dmitry
_______________________________________________ Ros-dev mailing list Ros-dev@reactos.org http://www.reactos.org/mailman/listinfo/ros-dev
But his reason was that in case with if ((a = b) && (c = d)) parenthesis managed to hide a compiler warning about equal statement being used instead of a comparison sign in the if.
On Feb 16, 2009, at 9:42 PM, Ged wrote:
IMO parenthesis should always be used, it helps to clarify what the developer who wrote the code really wanted.
I also find code which makes use of parenthesis much easier to read quickly, although that's probably just a personal preference.
Ged.
Ged schrieb:
IMO parenthesis should always be used, it helps to clarify what the developer who wrote the code really wanted.
In this case the exact opposite is the case. The braces would make you and the compiler think that the programmer really wanted to make an assignment inside the if .
Why would anyone want to make an assignment like this inside an if statement? This is just a one-off case of a silly programming error.
I would personally always use parenthesis here, but I've never made this mistake. If other people choose not to do then that's up to them. I don't think our style guideline needs to be, or even can be so strict.
Ged.
-----Original Message----- From: ros-dev-bounces@reactos.org [mailto:ros-dev-bounces@reactos.org] On Behalf Of Timo Kreuzer Sent: 16 February 2009 19:22 To: ReactOS Development List Subject: Re: [ros-dev] [ros-diffs] [dgorbachev] 39422: Fix bug #4129 in CmpGetNameControlBlock().
Ged schrieb:
IMO parenthesis should always be used, it helps to clarify what the developer who wrote the code really wanted.
In this case the exact opposite is the case. The braces would make you and the compiler think that the programmer really wanted to make an assignment inside the if .
_______________________________________________ Ros-dev mailing list Ros-dev@reactos.org http://www.reactos.org/mailman/listinfo/ros-dev
I agree with Dmitry on that matter. The brackets are bad. They only hide possible bugs. I don't see any advantage. They don't add anything for the compiler nor do they add anything to readability. IMO it's easier to read with less braces.
Timo
Aleksey Bragin schrieb:
Good, let's discuss it developers-wide then, and add a result into the (to become official) Coding Style Guidelines for kernel-mode code. The only thing I'm bothered with is to keep the style consistent across kernel (and drivers, where possible). And right now bracing arguments of && and || prevails.
I'm eager to listen to other dev's opinion on that topic. You just showed one disadvantage of such method. Are there any advantages?
WBR, Aleksey.
On Feb 16, 2009, at 8:47 PM, Dmitry Gorbachev wrote:
No, it's just a matter of coding style used in the kernel.
What is a logical basis for adopting this particular rule? Sometimes it is not just a matter of preference.
These parentheses are unnecessary for humans, and if they are not needed for compilers or some other tools, they are bad! Without them, the bug could be noticed earlier.
Cheers, Dmitry
Ros-dev mailing list Ros-dev@reactos.org http://www.reactos.org/mailman/listinfo/ros-dev
I agree with the requirement for the parenthesis, simply because it helps separate the various conditions that are being tested for. Even looking at the code without braces, you should be questioning exactly what is going on in there since you generally don't do assignments inside if statements. The fact that the compiler failed to catch it because of the parenthesis doesn't excuse a programmer from not seeing the problem. I personally would never assume that a dev intended to do an assignment just because he or she put parenthesis around such a statement, I would automatically assume they messed up typing and left off a = in the check.
2009/2/16 Timo Kreuzer timo.kreuzer@web.de
I agree with Dmitry on that matter. The brackets are bad. They only hide possible bugs. I don't see any advantage. They don't add anything for the compiler nor do they add anything to readability. IMO it's easier to read with less braces.
Timo
Aleksey Bragin schrieb:
Good, let's discuss it developers-wide then, and add a result into the (to become official) Coding Style Guidelines for kernel-mode code. The only thing I'm bothered with is to keep the style consistent across kernel (and drivers, where possible). And right now bracing arguments of && and || prevails.
I'm eager to listen to other dev's opinion on that topic. You just showed one disadvantage of such method. Are there any advantages?
WBR, Aleksey.
On Feb 16, 2009, at 8:47 PM, Dmitry Gorbachev wrote:
No, it's just a matter of coding style used in the kernel.
What is a logical basis for adopting this particular rule? Sometimes it is not just a matter of preference.
These parentheses are unnecessary for humans, and if they are not needed for compilers or some other tools, they are bad! Without them, the bug could be noticed earlier.
Cheers, Dmitry
Ros-dev mailing listRos-dev@reactos.orghttp://www.reactos.org/mailman/listinfo/ros-dev
Ros-dev mailing list Ros-dev@reactos.org http://www.reactos.org/mailman/listinfo/ros-dev
Zachary Gorden schrieb:
I agree with the requirement for the parenthesis, simply because it helps separate the various conditions that are being tested for. Even looking at
The conditions are clearly seperated by || or by &&. It should be instantly clear that the comparison is higher than || and && in the order of operations. Something like "if (a == (b || c) > d)" wouldn't make sense anyway.
Do not mess this up with bracing things like if ((a == 0 || a > 2) && b < 0) Although the braces are not needed, it makes sense to set them.
the code without braces, you should be questioning exactly what is going on in there since you generally don't do assignments inside if statements. The
You will probably find a lot of "if (!(ret = FooBar()) error();" here and there in our code. Maybe we should have a rule that you *must not* do that.
fact that the compiler failed to catch it because of the parenthesis doesn't excuse a programmer from not seeing the problem. I personally would never assume that a dev intended to do an assignment just because he or she put parenthesis around such a statement, I would automatically assume they messed up typing and left off a = in the check.
It's of cause the programmers fault. That doesn't make forcing a coding style that hides such bugs any better. This kind of error is quite common. The proposed rule will effectively prevent these typos from being detected when compiling your code.
I would agree on the rule if we could make the compiler always scream when someone does an assignment inside an if statement and we would prohibit this totally.
On Mon, Feb 16, 2009 at 12:54 PM, Timo Kreuzer timo.kreuzer@web.de wrote:
Zachary Gorden schrieb:
I agree with the requirement for the parenthesis, simply because it helps separate the various conditions that are being tested for. Even looking at
The conditions are clearly seperated by || or by &&. It should be instantly clear that the comparison is higher than || and && in the order of operations. Something like "if (a == (b || c) > d)" wouldn't make sense anyway.
Do not mess this up with bracing things like if ((a == 0 || a > 2) && b < 0) Although the braces are not needed, it makes sense to set them.
() -> parentheses [] -> brackets {} -> braces
The parentheses are not needed? You better check your C book for operator precedence. && has higher precedence than ||, so in this case the parentheses are needed. This is also why using parentheses is a good idea, because you never know if the original author has any idea about operator precedence. The use of parentheses makes the original intention clear to other authors.
Apart from all the other excellent points on why the current coding style (see Wiki) makes sense, let me offer one last piece of advice. I wrote 50% of the kernel code, and the other 30% is written by people whom I have tutored and taught our official coding style (for some reason it now seems to have become unofficial?!).
If you choose another coding style, you'll now have to re-write 80% of the kernel code to match it.
Best regards, Alex Ionescu
On Mon, Feb 16, 2009 at 4:14 PM, James Hawkins truiken@gmail.com wrote:
On Mon, Feb 16, 2009 at 12:54 PM, Timo Kreuzer timo.kreuzer@web.de wrote:
Zachary Gorden schrieb:
I agree with the requirement for the parenthesis, simply because it
helps
separate the various conditions that are being tested for. Even looking
at
The conditions are clearly seperated by || or by &&. It should be instantly clear that the comparison is higher than || and && in the order of operations. Something like "if (a == (b || c) > d)" wouldn't make sense anyway.
Do not mess this up with bracing things like if ((a == 0 || a > 2) && b < 0) Although the braces are not needed, it makes sense to set them.
() -> parentheses [] -> brackets {} -> braces
The parentheses are not needed? You better check your C book for operator precedence. && has higher precedence than ||, so in this case the parentheses are needed. This is also why using parentheses is a good idea, because you never know if the original author has any idea about operator precedence. The use of parentheses makes the original intention clear to other authors.
-- James Hawkins _______________________________________________ Ros-dev mailing list Ros-dev@reactos.org http://www.reactos.org/mailman/listinfo/ros-dev
James Hawkins schrieb:
On Mon, Feb 16, 2009 at 12:54 PM, Timo Kreuzer timo.kreuzer@web.de wrote:
Zachary Gorden schrieb:
I agree with the requirement for the parenthesis, simply because it helps separate the various conditions that are being tested for. Even looking at
The conditions are clearly seperated by || or by &&. It should be instantly clear that the comparison is higher than || and && in the order of operations. Something like "if (a == (b || c) > d)" wouldn't make sense anyway.
Do not mess this up with bracing things like if ((a == 0 || a > 2) && b < 0) Although the braces are not needed, it makes sense to set them.
() -> parentheses [] -> brackets {} -> braces
Hmm ok, good to know.
The parentheses are not needed? You better check your C book for operator precedence. && has higher precedence than ||, so in this case the parentheses are needed. This is also why using parentheses is a good idea, because you never know if the original author has any idea about operator precedence. The use of parentheses makes the original intention clear to other authors.
Sorry, of cause you are right. my fault. I initially wanted to write something different, got it messed up.. ;-)
James Hawkins wrote:
The parentheses are not needed? You better check your C book for operator precedence. && has higher precedence than ||, so in this case the parentheses are needed. This is also why using parentheses is a good idea, because you never know if the original author has any idea about operator precedence. The use of parentheses makes the original intention clear to other authors.
An absolutely perfect example which nicely backs up my original argument. I feel I now have nothing more to add. :)
Ged.
Ged schrieb:
James Hawkins wrote:
The parentheses are not needed? You better check your C book for operator precedence. && has higher precedence than ||, so in this case the parentheses are needed. This is also why using parentheses is a good idea, because you never know if the original author has any idea about operator precedence. The use of parentheses makes the original intention clear to other authors.
An absolutely perfect example which nicely backs up my original argument. I feel I now have nothing more to add. :)
Ok, but I have. The example that I fucked up, was an example where I wanted to say that parantheses are useful. Whereas I think in the case of || and && they should not be added as you can break a warning.
The arguments that have been brought up pro parenthesing are perfectly valid (except Alex' one maybe :)) when it comes to generic use of parentheses. But none of them is valid is this special case. If you want to generalize this ad infinitum, let's start adding parentheses around other things, too.
What about printf(Format, String + strlen(Prefix), i + 1); You better add some parentheses around (i + 3) so it won't be mistaken for printf(Format, String + (strlen(Prefix), i) + 1);
What about this:
(Callback->RoutineBlock).Object = NULL;
ExfReleaseRundownProtectionEx((&(CallbackRoutineBlock->RundownProtect)), ((Value.RefCnt) + 1)); NewValue.Object = (InterlockedCompareExchangePointer((&(FastRef->Object)),
(NewValue.Object),
(Value.Object))); for (((Entry = (Head->Flink)), (i = 0)); ((i < (Info->Depth)) && ((Entry->Flink) != NULL)); ((i++), (Entry = (Entry->Flink))))
Looks like crap, doesn't it?
You need to draw the line somewhere. IMO there's no use in adding parentheses around something when any different parenthesing wouldn't make sense. You cannot mistake if (a < 1 && b < 2) for if (a < (1 && b) < 2) as the second doesn't make sense.
Now I'd just like to see only one concrete example of when adding parentheses around comparisons seperated by || / && has an advantage over not adding them.
-
Aleksey Bragin -
Alex Ionescu -
Dmitry Gorbachev -
Ged -
James Hawkins -
Timo Kreuzer -
Zachary Gorden