* [PATCH] staging: vboxvideo: vbox_main: Remove unnecessary local variable
@ 2019-01-10 6:13 Sidong Yang
2019-01-10 12:23 ` Dan Carpenter
2019-01-11 9:42 ` Greg Kroah-Hartman
0 siblings, 2 replies; 6+ messages in thread
From: Sidong Yang @ 2019-01-10 6:13 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Sidong Yang, devel, linux-kernel
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 <realwakka@gmail.com>
---
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,
- &have_hints);
- if (ret)
+ if (hgsmi_query_conf(vbox->guest_pool,
+ VBOX_VBVA_CONF32_MODE_HINT_REPORTING,
+ &have_hints))
return false;
- ret = hgsmi_query_conf(vbox->guest_pool,
- VBOX_VBVA_CONF32_GUEST_CURSOR_REPORTING,
- &have_cursor);
- if (ret)
+ if (hgsmi_query_conf(vbox->guest_pool,
+ VBOX_VBVA_CONF32_GUEST_CURSOR_REPORTING,
+ &have_cursor))
return false;
return have_hints == VINF_SUCCESS && have_cursor == VINF_SUCCESS;
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] staging: vboxvideo: vbox_main: Remove unnecessary local variable
2019-01-10 6:13 [PATCH] staging: vboxvideo: vbox_main: Remove unnecessary local variable Sidong Yang
@ 2019-01-10 12:23 ` Dan Carpenter
2019-01-10 17:00 ` Sidong Yang
2019-01-11 9:42 ` Greg Kroah-Hartman
1 sibling, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2019-01-10 12:23 UTC (permalink / raw)
To: Sidong Yang; +Cc: Greg Kroah-Hartman, devel, linux-kernel
On Thu, Jan 10, 2019 at 06:13:47AM +0000, 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 <realwakka@gmail.com>
> ---
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] staging: vboxvideo: vbox_main: Remove unnecessary local variable
2019-01-10 12:23 ` Dan Carpenter
@ 2019-01-10 17:00 ` Sidong Yang
2019-01-10 19:44 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Sidong Yang @ 2019-01-10 17:00 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Greg Kroah-Hartman, devel, linux-kernel
On Thu, Jan 10, 2019 at 03:23:58PM +0300, Dan Carpenter wrote:
> On Thu, Jan 10, 2019 at 06:13:47AM +0000, 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 <realwakka@gmail.com>
> > ---
>
> 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
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] staging: vboxvideo: vbox_main: Remove unnecessary local variable
2019-01-10 17:00 ` Sidong Yang
@ 2019-01-10 19:44 ` Dan Carpenter
2019-01-11 3:36 ` Sidong Yang
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2019-01-10 19:44 UTC (permalink / raw)
To: Sidong Yang; +Cc: Greg Kroah-Hartman, devel, linux-kernel
On Thu, Jan 10, 2019 at 05:00:24PM +0000, 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] staging: vboxvideo: vbox_main: Remove unnecessary local variable
2019-01-10 19:44 ` Dan Carpenter
@ 2019-01-11 3:36 ` Sidong Yang
0 siblings, 0 replies; 6+ messages in thread
From: Sidong Yang @ 2019-01-11 3:36 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Greg Kroah-Hartman, devel, linux-kernel
On Thu, Jan 10, 2019 at 10:44:08PM +0300, Dan Carpenter wrote:
> On Thu, Jan 10, 2019 at 05:00:24PM +0000, 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: vboxvideo: vbox_main: Remove unnecessary local variable
2019-01-10 6:13 [PATCH] staging: vboxvideo: vbox_main: Remove unnecessary local variable Sidong Yang
2019-01-10 12:23 ` Dan Carpenter
@ 2019-01-11 9:42 ` Greg Kroah-Hartman
1 sibling, 0 replies; 6+ messages in thread
From: Greg Kroah-Hartman @ 2019-01-11 9:42 UTC (permalink / raw)
To: Sidong Yang; +Cc: devel, linux-kernel
On Thu, Jan 10, 2019 at 06:13:47AM +0000, 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 <realwakka@gmail.com>
> ---
> 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,
> - &have_hints);
> - if (ret)
> + if (hgsmi_query_conf(vbox->guest_pool,
> + VBOX_VBVA_CONF32_MODE_HINT_REPORTING,
> + &have_hints))
> return false;
As Dan says, the original is best here. I'm dropping this from my patch
queue.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-01-11 9:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-01-10 6:13 [PATCH] staging: vboxvideo: vbox_main: Remove unnecessary local variable Sidong Yang
2019-01-10 12:23 ` Dan Carpenter
2019-01-10 17:00 ` Sidong Yang
2019-01-10 19:44 ` Dan Carpenter
2019-01-11 3:36 ` Sidong Yang
2019-01-11 9:42 ` Greg Kroah-Hartman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox