Re: [PATCH linux-next] drm/gem: Remove the unneeded result variable

[Thierry Reding]

On Wed, Aug 31, 2022 at 12:25:52PM +0300, Jani Nikula wrote:
> On Wed, 31 Aug 2022, cgel.zte@xxxxxxxxx wrote:
> > From: ye xingchen <ye.xingchen@xxxxxxxxxx>
> >
> > Return the value drm_gem_handle_delete() directly instead of storing it
> > in another redundant variable.
> 
> Personally, I generally prefer keeping these types of variables, because
> they make future changes on the code easier. For example, adding a new
> call where you need to check the return value, or having to deal with
> the error value right there. Or just adding ad hoc debug logging of the
> return value.
> 
> Related, but not exactly the same, see commit b784c7707502
> ("coccinnelle: Remove ptr_ret script"). Adding Cc's from that commit for
> input.
> 
> BR,
> Jani.
> 
> > Reported-by: Zeal Robot <zealci@xxxxxxxxxx>
> > Signed-off-by: ye xingchen <ye.xingchen@xxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/drm_gem.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index ad068865ba20..3fa0deff3014 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -782,14 +782,11 @@ drm_gem_close_ioctl(struct drm_device *dev, void *data,
> >  		    struct drm_file *file_priv)
> >  {
> >  	struct drm_gem_close *args = data;
> > -	int ret;
> >  
> >  	if (!drm_core_check_feature(dev, DRIVER_GEM))
> >  		return -EOPNOTSUPP;
> >  
> > -	ret = drm_gem_handle_delete(file_priv, args->handle);
> > -
> > -	return ret;
> > +	return drm_gem_handle_delete(file_priv, args->handle);
> >  }
> >  
> >  /**

I agree with Jani here. If I recall correctly, the pattern that the
coccinelle script was dealing with was slightly different and in this
example would've looked like this.

	ret = drm_gem_handle_delete(file_priv, args->handle);
	if (ret)
		return ret;

	return 0;

You can collapse that in the same way, but I think it makes it even
clearer than the original why it makes sense to keep this construct.

If you even need to add any other cleanup code between the "return ret;"
and "return 0;" lines, you can do so without rearranging the rest of the
code.

It's obviously highly subjective which one is preferable, which is why
the recommendation at the time was not to provide a script that would
make any suggestions as to the "right" style.

In my opinion, if you write the code in the condensed form, that's fine,
but if you prefer the more verbose variant, that's equally fine. Trying
to "fix" this up after the fact for no other reason than to save a few
lines and/or a variable, however, is a complete waste of time.

Thierry

Attachment: signature.asc
Description: PGP signature