* [PATCH] drm/amdgpu: fix compare_const_fl.cocci warnings @ 2016-04-15 7:15 Julia Lawall 2016-04-15 8:46 ` Christian König 0 siblings, 1 reply; 5+ messages in thread From: Julia Lawall @ 2016-04-15 7:15 UTC (permalink / raw) To: Dave Airlie Cc: kbuild-all, Alex Deucher, Christian König, dri-devel, linux-kernel 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> --- 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( ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/amdgpu: fix compare_const_fl.cocci warnings 2016-04-15 7:15 [PATCH] drm/amdgpu: fix compare_const_fl.cocci warnings Julia Lawall @ 2016-04-15 8:46 ` Christian König 2016-04-15 14:20 ` Julia Lawall 0 siblings, 1 reply; 5+ messages in thread From: Christian König @ 2016-04-15 8:46 UTC (permalink / raw) To: Julia Lawall, Dave Airlie Cc: kbuild-all, Alex Deucher, dri-devel, linux-kernel 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. 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( ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/amdgpu: fix compare_const_fl.cocci warnings 2016-04-15 8:46 ` Christian König @ 2016-04-15 14:20 ` Julia Lawall 2016-04-15 15:14 ` Emil Velikov 2016-04-15 15:50 ` Joe Perches 0 siblings, 2 replies; 5+ messages in thread From: Julia Lawall @ 2016-04-15 14:20 UTC (permalink / raw) To: Christian König Cc: Julia Lawall, Dave Airlie, kbuild-all, Alex Deucher, dri-devel, linux-kernel, joe [-- Attachment #1: Type: TEXT/PLAIN, Size: 1707 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( > > ^ permalink raw reply [flat|nested] 5+ 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 2016-04-15 15:50 ` Joe Perches 1 sibling, 0 replies; 5+ 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] 5+ 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 @ 2016-04-15 15:50 ` Joe Perches 1 sibling, 0 replies; 5+ 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] 5+ messages in thread
end of thread, other threads:[~2016-04-15 15:50 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-04-15 7:15 [PATCH] drm/amdgpu: fix compare_const_fl.cocci warnings Julia Lawall 2016-04-15 8:46 ` Christian König 2016-04-15 14:20 ` Julia Lawall 2016-04-15 15:14 ` Emil Velikov 2016-04-15 15:50 ` Joe Perches
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).