[ros-diffs] [dgorbachev] 39422: Fix bug #4129 in CmpGetNameControlBlock().

[Dmitry Gorbachev]

>   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

[Aleksey Bragin]

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

[Ged]

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(a)reactos.org [mailto:[e-mail]] 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(a)reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev

[Aleksey Bragin]

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.

[Timo Kreuzer]

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 .

[Ged]

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(a)reactos.org [mailto:[e-mail]] 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(a)reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev

[Timo Kreuzer]

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(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev
>
>

[Zachary Gorden]

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(a)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 [e-mail]://www.reactos.org/mailman/listinfo/ros-dev
>
>
>
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev
>

[Timo Kreuzer]

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.

[James Hawkins]

On Mon, Feb 16, 2009 at 12:54 PM, Timo Kreuzer <timo.kreuzer(a)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

[Alex Ionescu]

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(a)gmail.com> wrote:

>   On Mon, Feb 16, 2009 at 12:54 PM, Timo Kreuzer
> <timo.kreuzer(a)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(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev
>

[Timo Kreuzer]

James Hawkins schrieb:
>   On Mon, Feb 16, 2009 at 12:54 PM, Timo Kreuzer
> <timo.kreuzer(a)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.. ;-)

[Ged]

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.

[Timo Kreuzer]

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.