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 <[email protected]>
---
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
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 <[email protected]>
> ---
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
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 <[email protected]>
> > ---
>
> 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
>
>
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
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
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 <[email protected]>
> ---
> 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