* Re: [PATCH] drm/amdgpu: fix compare_const_fl.cocci warnings
@ 2016-04-15 14:20 ` Julia Lawall
0 siblings, 0 replies; 10+ messages in thread
From: Julia Lawall @ 2016-04-15 14:20 UTC (permalink / raw)
To: Christian König
Cc: joe, linux-kernel, dri-devel, Julia Lawall, kbuild-all,
Alex Deucher, Dave Airlie
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1759 bytes --]
On Fri, 15 Apr 2016, Christian König wrote:
> Am 15.04.2016 um 09:15 schrieb Julia Lawall:
> > Move constants to the right of binary operators.
> >
> > Generated by: scripts/coccinelle/misc/compare_const_fl.cocci
> >
> > Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
> > Signed-off-by: Julia Lawall <julia.lawall@lip6.fr>
>
> In general the patch looks ok, but do we have a documented preference where to
> place constants in the coding style docs?
>
> While it's not so much of a problem any more with modern compilers, some
> people still prefer to have it on the left side to catch accidental value
> assignments.
I don't know if it is documented. Joe Perches suggested that on the right
was better in general - maybe he knows if this is written somewhere.
There are 504 occurrences of NULL == in the kernel, and 19524 occurrences
of == NULL.
julia
>
> Regards,
> Christian.
>
> > ---
> >
> > Could be nice to put the thing being tested first.
> >
> > amdgpu_grph_object_id_helpers.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_grph_object_id_helpers.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_grph_object_id_helpers.c
> > @@ -169,11 +169,11 @@ struct graphics_object_id amdgpu_object_
> > struct graphics_object_id go_id = { 0 };
> > type = object_type_from_bios_object_id(bios_object_id);
> > - if (OBJECT_TYPE_UNKNOWN == type)
> > + if (type == OBJECT_TYPE_UNKNOWN)
> > return go_id;
> > enum_id = enum_id_from_bios_object_id(bios_object_id);
> > - if (ENUM_ID_UNKNOWN == enum_id)
> > + if (enum_id == ENUM_ID_UNKNOWN)
> > return go_id;
> > go_id = display_graphics_object_id_init(
>
>
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] drm/amdgpu: fix compare_const_fl.cocci warnings
2016-04-15 14:20 ` Julia Lawall
@ 2016-04-15 15:14 ` Emil Velikov
-1 siblings, 0 replies; 10+ messages in thread
From: Emil Velikov @ 2016-04-15 15:14 UTC (permalink / raw)
To: Julia Lawall
Cc: Christian König, Joe Perches,
Linux-Kernel@Vger. Kernel. Org, ML dri-devel, kbuild-all,
Alex Deucher, Dave Airlie
On 15 April 2016 at 15:20, Julia Lawall <julia.lawall@lip6.fr> wrote:
> On Fri, 15 Apr 2016, Christian König wrote:
>> Am 15.04.2016 um 09:15 schrieb Julia Lawall:
>> > Move constants to the right of binary operators.
>> >
>> > Generated by: scripts/coccinelle/misc/compare_const_fl.cocci
>> >
>> > Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
>> > Signed-off-by: Julia Lawall <julia.lawall@lip6.fr>
>>
>> In general the patch looks ok, but do we have a documented preference where to
>> place constants in the coding style docs?
>>
>> While it's not so much of a problem any more with modern compilers, some
>> people still prefer to have it on the left side to catch accidental value
>> assignments.
>
> I don't know if it is documented. Joe Perches suggested that on the right
> was better in general - maybe he knows if this is written somewhere.
>
> There are 504 occurrences of NULL == in the kernel, and 19524 occurrences
> of == NULL.
>
To throw in some more numbers:
>From drivers/gpu/drm/amd/ - ~40 for "NULL *== *" and ~400 for " *== *NULL" ;-)
-Emil
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/amdgpu: fix compare_const_fl.cocci warnings
@ 2016-04-15 15:14 ` Emil Velikov
0 siblings, 0 replies; 10+ messages in thread
From: Emil Velikov @ 2016-04-15 15:14 UTC (permalink / raw)
To: Julia Lawall
Cc: Linux-Kernel@Vger. Kernel. Org, ML dri-devel, Alex Deucher,
kbuild-all, Joe Perches, Dave Airlie, Christian König
On 15 April 2016 at 15:20, Julia Lawall <julia.lawall@lip6.fr> wrote:
> On Fri, 15 Apr 2016, Christian König wrote:
>> Am 15.04.2016 um 09:15 schrieb Julia Lawall:
>> > Move constants to the right of binary operators.
>> >
>> > Generated by: scripts/coccinelle/misc/compare_const_fl.cocci
>> >
>> > Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
>> > Signed-off-by: Julia Lawall <julia.lawall@lip6.fr>
>>
>> In general the patch looks ok, but do we have a documented preference where to
>> place constants in the coding style docs?
>>
>> While it's not so much of a problem any more with modern compilers, some
>> people still prefer to have it on the left side to catch accidental value
>> assignments.
>
> I don't know if it is documented. Joe Perches suggested that on the right
> was better in general - maybe he knows if this is written somewhere.
>
> There are 504 occurrences of NULL == in the kernel, and 19524 occurrences
> of == NULL.
>
To throw in some more numbers:
From drivers/gpu/drm/amd/ - ~40 for "NULL *== *" and ~400 for " *== *NULL" ;-)
-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/amdgpu: fix compare_const_fl.cocci warnings
2016-04-15 14:20 ` Julia Lawall
@ 2016-04-15 15:50 ` Joe Perches
-1 siblings, 0 replies; 10+ messages in thread
From: Joe Perches @ 2016-04-15 15:50 UTC (permalink / raw)
To: Julia Lawall, Christian König
Cc: Dave Airlie, kbuild-all, Alex Deucher, dri-devel, linux-kernel
On Fri, 2016-04-15 at 16:20 +0200, Julia Lawall wrote:
> On Fri, 15 Apr 2016, Christian König wrote:
> > Am 15.04.2016 um 09:15 schrieb Julia Lawall:
> > > Move constants to the right of binary operators.
> > >
> > > Generated by: scripts/coccinelle/misc/compare_const_fl.cocci
> > >
> > > Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
> > > Signed-off-by: Julia Lawall <julia.lawall@lip6.fr>
> >
> > In general the patch looks ok, but do we have a documented preference where to
> > place constants in the coding style docs?
> >
> > While it's not so much of a problem any more with modern compilers, some
> > people still prefer to have it on the left side to catch accidental value
> > assignments.
>
> I don't know if it is documented. Joe Perches suggested that on the right
> was better in general - maybe he knows if this is written somewhere.
>
> There are 504 occurrences of NULL == in the kernel, and 19524 occurrences
> of == NULL.
A long time ago Linus wrote:
> On Wed, 2004-03-10 at 18:36, Linus Torvalds wrote:
> > > The kind of bug that the "0 == x" syntax protects against
> > > is LESS LIKELY to happen than the kind of bug it tends to cause.
> >
> > Not my experience. I'd personally prefer a stated preference for.
> >
> > (lvalue == rvalue) vs (rvalue == lvalue)
>
> The thing is, your "vs" above makes no sense.
>
> Quite often, both sides are rvalues, or lvalues. In fact, often you may
> not even know from a simple syntactic look which one either side is, since
> they can be macros etc.
>
> So asking for consistency is just impossible, because the exact same
> expression may semantically parse as either depending on things like
> macros that have architectural dependencies.
>
> So the rule would have to be something like this:
> - if one side is _obviously_ a lvalue, and the other side is _obviously_
> a rvalue, then do X.
>
> That kind of rule makes very little sense, but if you want a stated
> preference, then my preference is to say that in that obvious case, the
> lvalue comes on the left side, and the rvalue comes on the right side.
>
> Why? Because that is literally how people think. People have been taught
> since before first grade to think "if I have 8 apples, and I give Tom one
> apple, how many apples do I have".
>
> Notice how I didn't say "if 8 applies is what I have.."
>
> The reason for "if (x == 8)" comes from the way we're taught to think.
> Arguing against that _fact_ is just totally non-productive, and you have
> to _force_ yourself to write it the other way around.
>
> And that just means that you will do other mistakes. You'll spend your
> time thinking about trying to express your conditionals in strange ways,
> and then not think about the _real_ issue.
>
> So let's make a few rules:
>
> - write your logical expressions the way people EXPECT them to be
> written. No silly rules that make no sense.
>
> Ergo:
>
> if (x == 8)
>
> is the ONE AND ONLY SANE WAY.
>
> - avoid using assignment inside logical expressions unless you have a
> damn good reason to.
>
> Ergo: write
>
> error = myfunction(xxxx)
> if (error) {
> ...
>
> instead of writing
>
> if (error = myfunction(xxxx))
> ....
>
> which is just unreadable and stupid.
>
> - Don't get hung about stupid rules.
>
> Ergo: sometimes assignments in conditionals make sense, especially in
> loops. Don't avoid them just because of some silly rule. But strive to
> use an explicit equality test when you do so:
>
> while ((a = function(b)) != 0)
> ...
>
> is fine.
>
> - The compiler warns about the mistakes that remain, if you follow these
> rules.
>
> - mistakes happen. Deal with it. Having tons of rules just makes them
> more likely. Expect mistakes, and make sure they are fixed quickly.
>
> Is that "stated preference" enough?
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] drm/amdgpu: fix compare_const_fl.cocci warnings
@ 2016-04-15 15:50 ` Joe Perches
0 siblings, 0 replies; 10+ messages in thread
From: Joe Perches @ 2016-04-15 15:50 UTC (permalink / raw)
To: Julia Lawall, Christian König
Cc: Alex Deucher, Dave Airlie, kbuild-all, dri-devel, linux-kernel
On Fri, 2016-04-15 at 16:20 +0200, Julia Lawall wrote:
> On Fri, 15 Apr 2016, Christian König wrote:
> > Am 15.04.2016 um 09:15 schrieb Julia Lawall:
> > > Move constants to the right of binary operators.
> > >
> > > Generated by: scripts/coccinelle/misc/compare_const_fl.cocci
> > >
> > > Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
> > > Signed-off-by: Julia Lawall <julia.lawall@lip6.fr>
> >
> > In general the patch looks ok, but do we have a documented preference where to
> > place constants in the coding style docs?
> >
> > While it's not so much of a problem any more with modern compilers, some
> > people still prefer to have it on the left side to catch accidental value
> > assignments.
>
> I don't know if it is documented. Joe Perches suggested that on the right
> was better in general - maybe he knows if this is written somewhere.
>
> There are 504 occurrences of NULL == in the kernel, and 19524 occurrences
> of == NULL.
A long time ago Linus wrote:
> On Wed, 2004-03-10 at 18:36, Linus Torvalds wrote:
> > > The kind of bug that the "0 == x" syntax protects against
> > > is LESS LIKELY to happen than the kind of bug it tends to cause.
> >
> > Not my experience. I'd personally prefer a stated preference for.
> >
> > (lvalue == rvalue) vs (rvalue == lvalue)
>
> The thing is, your "vs" above makes no sense.
>
> Quite often, both sides are rvalues, or lvalues. In fact, often you may
> not even know from a simple syntactic look which one either side is, since
> they can be macros etc.
>
> So asking for consistency is just impossible, because the exact same
> expression may semantically parse as either depending on things like
> macros that have architectural dependencies.
>
> So the rule would have to be something like this:
> - if one side is _obviously_ a lvalue, and the other side is _obviously_
> a rvalue, then do X.
>
> That kind of rule makes very little sense, but if you want a stated
> preference, then my preference is to say that in that obvious case, the
> lvalue comes on the left side, and the rvalue comes on the right side.
>
> Why? Because that is literally how people think. People have been taught
> since before first grade to think "if I have 8 apples, and I give Tom one
> apple, how many apples do I have".
>
> Notice how I didn't say "if 8 applies is what I have.."
>
> The reason for "if (x == 8)" comes from the way we're taught to think.
> Arguing against that _fact_ is just totally non-productive, and you have
> to _force_ yourself to write it the other way around.
>
> And that just means that you will do other mistakes. You'll spend your
> time thinking about trying to express your conditionals in strange ways,
> and then not think about the _real_ issue.
>
> So let's make a few rules:
>
> - write your logical expressions the way people EXPECT them to be
> written. No silly rules that make no sense.
>
> Ergo:
>
> if (x == 8)
>
> is the ONE AND ONLY SANE WAY.
>
> - avoid using assignment inside logical expressions unless you have a
> damn good reason to.
>
> Ergo: write
>
> error = myfunction(xxxx)
> if (error) {
> ...
>
> instead of writing
>
> if (error = myfunction(xxxx))
> ....
>
> which is just unreadable and stupid.
>
> - Don't get hung about stupid rules.
>
> Ergo: sometimes assignments in conditionals make sense, especially in
> loops. Don't avoid them just because of some silly rule. But strive to
> use an explicit equality test when you do so:
>
> while ((a = function(b)) != 0)
> ...
>
> is fine.
>
> - The compiler warns about the mistakes that remain, if you follow these
> rules.
>
> - mistakes happen. Deal with it. Having tons of rules just makes them
> more likely. Expect mistakes, and make sure they are fixed quickly.
>
> Is that "stated preference" enough?
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread