On Mon, Jan 16, 2017 at 11:40:10PM +0000, Eric Engestrom wrote:
> > diff --git a/drivers/gpu/drm/vc4/vc4_render_cl.c
> > b/drivers/gpu/drm/vc4/vc4_render_cl.c
> > index 08886a3..5cdd003 100644
> > --- a/drivers/gpu/drm/vc4/vc4_render_cl.c
> > +++ b/drivers/gpu/drm/vc4/vc4_render_cl.c
> > @@ -461,7 +461,7 @@ static int vc4_rcl_surface_setup(struct vc4_exec_info
> > *exec,
> > }
> >
> > ret = vc4_full_res_bounds_check(exec, *obj, surf);
> > - if (!ret)
> > + if (ret)
> > return ret;
> >
> > return 0;
>
> This now boils down to `return vc4_full_res_bounds_check(...);`, so you
> could get rid of the `ret` variable completely :)
I tried to leave it the original style which the author intended.
That's also my prefered style. I like big chunky "return 0;". I
actually found this bug by looking at places where people return a
variable where a literal would work:
if (!ret)
return ret;
It's ambiguos if they intended to return a negative, or if they
condition is reversed. The other reason why I slightly prefer his style
is because people get so "clever" with the last condition in a function
and it drives me nuts. They'll do a series of checks like this:
if (fail)
goto;
if (fail)
goto;
if (fail)
goto;
if (success)
return;
label:
We should be testing for failure generally, but this particular kind of
success check is like nails on a chalk board for me. My younger self
is guilty of this cleverness as well....
Of course, the other way: "return vc4_full_res_bounds_check();" is
fine too. It's not something that bothers me. I guess I just would
do whatever the original author prefers.
regards,
dan carpenter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
