Re: [PATCH] staging: vboxvideo: vbox_main: Remove unnecessary local variable
On Thu, Jan 10, 2019 at 06:13:47AM +, Sidong Yang wrote: > Removed unnecessary local variable in have_hgsmi_mode_hints. > The result of hgsmi_query_conf should be directly compared without > assigning to local variable. > > Signed-off-by: Sidong Yang > --- > drivers/staging/vboxvideo/vbox_main.c | 15 ++- > 1 file changed, 6 insertions(+), 9 deletions(-) > > diff --git a/drivers/staging/vboxvideo/vbox_main.c > b/drivers/staging/vboxvideo/vbox_main.c > index e1fb70a42d32..62a69fde7435 100644 > --- a/drivers/staging/vboxvideo/vbox_main.c > +++ b/drivers/staging/vboxvideo/vbox_main.c > @@ -170,18 +170,15 @@ static void vbox_accel_fini(struct vbox_private *vbox) > static bool have_hgsmi_mode_hints(struct vbox_private *vbox) > { > u32 have_hints, have_cursor; > - int ret; > > - ret = hgsmi_query_conf(vbox->guest_pool, > -VBOX_VBVA_CONF32_MODE_HINT_REPORTING, > -_hints); > - if (ret) > + if (hgsmi_query_conf(vbox->guest_pool, > + VBOX_VBVA_CONF32_MODE_HINT_REPORTING, > + _hints)) > return false; As Dan says, the original is best here. I'm dropping this from my patch queue. thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: vboxvideo: vbox_main: Remove unnecessary local variable
On Thu, Jan 10, 2019 at 10:44:08PM +0300, Dan Carpenter wrote: > On Thu, Jan 10, 2019 at 05:00:24PM +, Sidong Yang wrote: > > I think you just point out that my code isn't obvious because the > > function returns negative error codes. I agree with you. But what if > > change my code like if(hgsmi_query_conf() != 0). > > > > That's even worse! :P > > You should do comparisons with zero when you are talking about zero > meaning the number zero. In this case, hgsmi_query_conf() returns "zezro > meaning success" not "zero meaning the number zero". How many bytes? > Zero. That is the number zero. > > != zero is a double negative, because NOT and ZERO are negatives. If > double negatives simplified the code we would add four of them instead > of just the one: > > if hgsmi_query_conf() != 0) != 0) != 0) != 0) { > > See? Adding != 0 doesn't make it simpler... > > The other place where != 0 is appropriate besides talking about the > number is when you're using a strcmp() function because it works like > this: > > if (strcmp(a, b) < 0) { <-- means a < b > if (strcmp(a, b) == 0) { <-- means a == b > if (strcmp(a, b) != 0) { <-- means a != b > > regards, > dan carpenter > You're right. that is even worse. I understand and thank you for pointing out. regards, Sidong Yang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: vboxvideo: vbox_main: Remove unnecessary local variable
On Thu, Jan 10, 2019 at 05:00:24PM +, Sidong Yang wrote: > I think you just point out that my code isn't obvious because the > function returns negative error codes. I agree with you. But what if > change my code like if(hgsmi_query_conf() != 0). > That's even worse! :P You should do comparisons with zero when you are talking about zero meaning the number zero. In this case, hgsmi_query_conf() returns "zezro meaning success" not "zero meaning the number zero". How many bytes? Zero. That is the number zero. != zero is a double negative, because NOT and ZERO are negatives. If double negatives simplified the code we would add four of them instead of just the one: if hgsmi_query_conf() != 0) != 0) != 0) != 0) { See? Adding != 0 doesn't make it simpler... The other place where != 0 is appropriate besides talking about the number is when you're using a strcmp() function because it works like this: if (strcmp(a, b) < 0) { <-- means a < b if (strcmp(a, b) == 0) { <-- means a == b if (strcmp(a, b) != 0) { <-- means a != b regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: vboxvideo: vbox_main: Remove unnecessary local variable
On Thu, Jan 10, 2019 at 03:23:58PM +0300, Dan Carpenter wrote: > On Thu, Jan 10, 2019 at 06:13:47AM +, Sidong Yang wrote: > > Removed unnecessary local variable in have_hgsmi_mode_hints. > > The result of hgsmi_query_conf should be directly compared without > > assigning to local variable. > > > > Signed-off-by: Sidong Yang > > --- > > I sort of prefer the original... > > The hgsmi_query_conf() function returns negative error codes if it > can't complete the query because of allocation failures. To me that's > more obvious, when we write it in the original way. > > In the new code it looks like it returns bool or something. The > copy_to/from_user() are normally written like if (copy_to_user()) { > but those don't return negative error codes so it's a different > situation. > Hi, Dan. I think you just point out that my code isn't obvious because the function returns negative error codes. I agree with you. But what if change my code like if(hgsmi_query_conf() != 0). > This isn't something in checkpatch or CodingStyle so there isn't a > standard. It's just personal opinion vs personal opinion. If you were > going to do a lot of vboxvideo development, then it would be your > opinion which matters the most because you are doing the work. But > this is your first vboxvideo patch... > > Let's just leave it as-is. I agree with this comment. I'm just a newbie for this module and It isn't about checkpatch or CodingStyle. but I just wondered if my code has problem. regards, Sidong Yang > > regards, > dan carpenter > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: vboxvideo: vbox_main: Remove unnecessary local variable
On Thu, Jan 10, 2019 at 06:13:47AM +, Sidong Yang wrote: > Removed unnecessary local variable in have_hgsmi_mode_hints. > The result of hgsmi_query_conf should be directly compared without > assigning to local variable. > > Signed-off-by: Sidong Yang > --- I sort of prefer the original... The hgsmi_query_conf() function returns negative error codes if it can't complete the query because of allocation failures. To me that's more obvious, when we write it in the original way. In the new code it looks like it returns bool or something. The copy_to/from_user() are normally written like if (copy_to_user()) { but those don't return negative error codes so it's a different situation. This isn't something in checkpatch or CodingStyle so there isn't a standard. It's just personal opinion vs personal opinion. If you were going to do a lot of vboxvideo development, then it would be your opinion which matters the most because you are doing the work. But this is your first vboxvideo patch... Let's just leave it as-is. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: vboxvideo: vbox_main: Remove unnecessary local variable
Removed unnecessary local variable in have_hgsmi_mode_hints. The result of hgsmi_query_conf should be directly compared without assigning to local variable. Signed-off-by: Sidong Yang --- drivers/staging/vboxvideo/vbox_main.c | 15 ++- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/drivers/staging/vboxvideo/vbox_main.c b/drivers/staging/vboxvideo/vbox_main.c index e1fb70a42d32..62a69fde7435 100644 --- a/drivers/staging/vboxvideo/vbox_main.c +++ b/drivers/staging/vboxvideo/vbox_main.c @@ -170,18 +170,15 @@ static void vbox_accel_fini(struct vbox_private *vbox) static bool have_hgsmi_mode_hints(struct vbox_private *vbox) { u32 have_hints, have_cursor; - int ret; - ret = hgsmi_query_conf(vbox->guest_pool, - VBOX_VBVA_CONF32_MODE_HINT_REPORTING, - _hints); - if (ret) + if (hgsmi_query_conf(vbox->guest_pool, +VBOX_VBVA_CONF32_MODE_HINT_REPORTING, +_hints)) return false; - ret = hgsmi_query_conf(vbox->guest_pool, - VBOX_VBVA_CONF32_GUEST_CURSOR_REPORTING, - _cursor); - if (ret) + if (hgsmi_query_conf(vbox->guest_pool, +VBOX_VBVA_CONF32_GUEST_CURSOR_REPORTING, +_cursor)) return false; return have_hints == VINF_SUCCESS && have_cursor == VINF_SUCCESS; -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel