linux-kernel - Re: [PATCH] drm/amdgpu: fix compare_const

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1460735428.19090.23.camel@perches.com>
Date:	Fri, 15 Apr 2016 08:50:28 -0700
From:	Joe Perches <joe@...ches.com>
To:	Julia Lawall <julia.lawall@...6.fr>,
	Christian König <christian.koenig@....com>
Cc:	Dave Airlie <airlied@...hat.com>, kbuild-all@...org,
	Alex Deucher <alexander.deucher@....com>,
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/amdgpu: fix compare_const_fl.cocci warnings

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@...el.com>
> > > Signed-off-by: Julia Lawall <julia.lawall@...6.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?
>