drm/amdgpu: fix compare_const_fl.cocci warnings

Commit Message

Julia Lawall April 15, 2016, 7:15 a.m. UTC

Move constants to the right of binary operators.

Generated by: scripts/coccinelle/misc/compare_const_fl.cocci

Signed-off-by: Fengguang Wu <[e-mail]>
Signed-off-by: Julia Lawall <[e-mail]>
---

Could be nice to put the thing being tested first.

 amdgpu_grph_object_id_helpers.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Christian König April 15, 2016, 8:46 a.m. UTC | #1

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 <[e-mail]>
> Signed-off-by: Julia Lawall <[e-mail]>

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.

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(

Julia Lawall April 15, 2016, 2:20 p.m. UTC | #2

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 <[e-mail]>
> > Signed-off-by: Julia Lawall <[e-mail]>
>
> 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(
>
>

Emil Velikov April 15, 2016, 3:14 p.m. UTC | #3

On 15 April 2016 at 15:20, Julia Lawall <[e-mail]> 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 <[e-mail]>
>> > Signed-off-by: Julia Lawall <[e-mail]>
>>
>> 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

Joe Perches April 15, 2016, 3:50 p.m. UTC | #4

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 <[e-mail]>
> > > Signed-off-by: Julia Lawall <[e-mail]>
> >
> > 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?
>

Patch

--- 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(