Message ID | alpine.DEB.2.02.1604150914040.2061@localhost6.localdomain6 (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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(
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( > >
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
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? >
--- 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(