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 -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
