Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 31 Aug 2022 14:02:54 +0200
From:   Thierry Reding <treding@nvidia.com>
To:     Jani Nikula <jani.nikula@linux.intel.com>
Cc:     cgel.zte@gmail.com, airlied@linux.ie, tzimmermann@suse.de,
        ye xingchen <ye.xingchen@zte.com.cn>,
        Zeal Robot <zealci@zte.com.cn>, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org, Maxime Ripard <maxime@cerno.tech>,
        Julia Lawall <julia.lawall@inria.fr>,
        Wolfram Sang <wsa+renesas@sang-engineering.com>,
        Mark Brown <broonie@kernel.org>
Subject: Re: [PATCH linux-next] drm/gem: Remove the unneeded result variable
Message-ID: <Yw9N7lMunu9XjXo5@orome>
References: <20220831033301.302121-1-ye.xingchen@zte.com.cn>
 <8735dcepcv.fsf@intel.com>
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="wwaXRVMgg64Q3zs8"
Content-Disposition: inline
In-Reply-To: <8735dcepcv.fsf@intel.com>
User-Agent: Mutt/2.2.7 (2022-08-07)
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?NbEOrjJ6zYf1RyVqdVqSx2dHimsilhj96EvsHUtWoCygJDJFAJm8/u5xnzqF?=
 =?us-ascii?Q?q+FS2HWD197LJrh/32qY4jJtsQA8NuHpZ8phjvgTi/ekHz4HuLE1Mf/ECrsJ?=
 =?us-ascii?Q?bhkawjFXMtyi9OtHgQrU3jXRzt9PqvrZpiJD0Aehl89j4x2A2h5ItU646xUm?=
 =?us-ascii?Q?6zDoMp6Y/6iPdReNT4UCtWgvhnRPDjhI45BAo9mpSQ8Z8Jjx5ANPirzSOUvW?=
 =?us-ascii?Q?7jObiDRi8LCfovsoM7AiBuBDNwndDY7jQhTa5UAffOxkvo3QMoVcCfqfeTMU?=
 =?us-ascii?Q?OsAVI12Qf3mGsgF7kRGtzMG8EARqC5UDnPKxhMukWME5fhbO4J7yypcaYbp1?=
 =?us-ascii?Q?Ajne1paNweLhZamuRjWRM3IPLm6c5C/sLbkhuSobWtYy3sOU6/UFIFH5r08c?=
 =?us-ascii?Q?hifYNEriqj7xphl5lJ4FAyH2d1UxZ2oNBEeMDAgNddeCK10rymAWqiFtNum/?=
 =?us-ascii?Q?0LE3nG//3kWX/BAu5+z+mmAXpqf0w+V+CGB6BvLLIsOYdS2/A0z/tUlEMjQi?=
 =?us-ascii?Q?y1z9QnI5nDeksF/sNRFEWxYh4PUVgBhZ9RZ5KGR+ALwyL71CRixVpy/J1/Zb?=
 =?us-ascii?Q?QHSJK+ihQRXTzM0GI7GGz+r4iGrOJWqUumqug8D6pBJVWawyi2NdV3dS5C60?=
 =?us-ascii?Q?uLjWWhEaLTlAdE2z1/+WmdHk1fjFc+4OglcctoSy43VLQi/wT3LY9ygMDdy/?=
 =?us-ascii?Q?9gaqhnkx8QLL+hMX2nVLRlUEMfLq/f1K2/+qF1iZ9OMKsfX5XKHIeD5rabL6?=
 =?us-ascii?Q?JZC9Zf0X/YdA4aVHu3gCfoXpgniSjEhokpz0mdUgRKA3uGLRoy8MP9JnK4Vy?=
 =?us-ascii?Q?O+pEy5rhmaOSDG59J+ms76W+FtXUi+m8YjJdQLjY11l4F15Hvrpj1tuybZh5?=
 =?us-ascii?Q?XizFq8CkPwFwjwE+UVCSL3oUAxkleUyEvsQUv9cZ52dvie56tl6fsaqCXYoF?=
 =?us-ascii?Q?7HmlOaxodLXnXugagkc0JTqhc9EBhfexLK8fuHQgMseulTGCqyIeGTgFWB59?=
 =?us-ascii?Q?yb6aCJk4+T9t8/JXJnBIx7oABOYVNse/fbcqhvZliRhcyBcSIOjsxEER5uMJ?=
 =?us-ascii?Q?4zNQwGRTZ8GCmwVRQwEoxYQwyrK0/dGWKbF3mbhtyJpsqimcLlxWxAb7doI4?=
 =?us-ascii?Q?GycuSPXU8HRGK7Zp4yPcfdFqZXH/a9GeSzh+Gsn5fUnYDhEQd/IwUhStH6Lz?=
 =?us-ascii?Q?hkMCIDTgpGeo3Ous9CXeyM/9J98bI1432gDb8TeEAr/qRMYUnyaM6/+dj2wV?=
 =?us-ascii?Q?DW7ACnx/O6BuP8fQ9zxQ8jdywdKopaUEJVrYf5r1hpane0O58KWXkENfQ0t/?=
 =?us-ascii?Q?/1yAYAqzazN6IhqY2N6+PrZdmIKnodfeUYZ/Nx+Brc8nd/CdBFZ8G/UPlidd?=
 =?us-ascii?Q?GcF9GulvGkzG1eydtuhPPdYCt3wii4zaRKk5482LHQaOYAZp8PALAVQOoe43?=
 =?us-ascii?Q?KNPbeO4IQC5A/JEz4/N0opbn3kfSwXjWoms1npcGqZVnUlLAL7B17Hd18WDw?=
 =?us-ascii?Q?SJ3PANtw2I92WqxoiLXTWkv1ZbOS9lFGXzU20wAfhKjNCDBOewJADqC49L7/?=
 =?us-ascii?Q?/H4zpvueFaOCmTTsGZ/51p4SAR1foVc+Zwby7x23gdgQfGBF1wf+R4JkhbgA?=
 =?us-ascii?Q?v9a94dUNZz9VjFbpbhGvxx1tg9EWGHQGvrVbsi5y4CJL?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 57d89091-e577-4b63-e5ab-08da8b48c15a
X-MS-Exchange-CrossTenant-AuthSource: DM8PR12MB5413.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Aug 2022 12:03:02.6243
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 2k8UM5undshxXg75xPUZg5ycVy1odqFqdVpzYNiiqUK0y7B7SxOhGOj0ukpjN/XEF/RnkstwTni9Bsjn1KyXig==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR12MB5603
X-Spam-Status: No, score=-1.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

--wwaXRVMgg64Q3zs8
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Aug 31, 2022 at 12:25:52PM +0300, Jani Nikula wrote:
> On Wed, 31 Aug 2022, cgel.zte@gmail.com wrote:
> > From: ye xingchen <ye.xingchen@zte.com.cn>
> >
> > Return the value drm_gem_handle_delete() directly instead of storing it
> > in another redundant variable.
>=20
> 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.
>=20
> Related, but not exactly the same, see commit b784c7707502
> ("coccinnelle: Remove ptr_ret script"). Adding Cc's from that commit for
> input.
>=20
> BR,
> Jani.
>=20
> > Reported-by: Zeal Robot <zealci@zte.com.cn>
> > Signed-off-by: ye xingchen <ye.xingchen@zte.com.cn>
> > ---
> >  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 =3D data;
> > -	int ret;
> > =20
> >  	if (!drm_core_check_feature(dev, DRIVER_GEM))
> >  		return -EOPNOTSUPP;
> > =20
> > -	ret =3D drm_gem_handle_delete(file_priv, args->handle);
> > -
> > -	return ret;
> > +	return drm_gem_handle_delete(file_priv, args->handle);
> >  }
> > =20
> >  /**

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 =3D 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

--wwaXRVMgg64Q3zs8
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAmMPTe4ACgkQ3SOs138+
s6EB/hAAvy/t9LLfH8ZoYSIwzY3MtjeHZY7eOQ5rbBcGXdrhP8XhP0npQlcSEYm6
GqaasiB1ZHfp051K3E/EVnoHUnyXW2bT3mxZFPVojx/bLyFaCiXBmtUb/mPkykz3
7umvaIU527qXRIM0NykHcyICs9NJpcCXtPBGq/ZXts2sBmjqNZrZZlFPkxIihaID
o0pd2GIV4a4PoUuyS4dvddFSn38T02czw4FfjaeGD3kxUgh0Cak19iDj6KRqDYKa
75+ZE8j83eUPK1tFsao7YAhQAUcxeqXwCg+n6IR0rkYztvfwA75GR6zLTjWGrlgo
sqP0PT0tgr1o5+6y7DjAmMPYIzEgERaHCgQTDK7P9+ez1gEBWgwgRqu2LAgkjtvP
8zv3utfECT6ZnbOht4fNcBPtkC9DKa0aWtIbtpO1Jd/g8m7wGdS4xRA7tBvzw+bQ
6nDgzDK44ezAFHjc3STdXZscq/yYZrJhbcV0IIUNfAZEAQ28DIkvilphF4lt2MxO
76xXu2VwoF4gH3KLgf60L2VI+eZAVof+F9cAnjiNO1Q0TgztXgupNGacNWpusQwU
2zhZr6JuoEcMAc3xZBYDtCoeGFHLDhrsOsfjhwwaB4ZEL8cJ/YfYqC7oMptmZZxe
gjRR+EKoMMueUs8+EUey/jIIhAouoRiYo1EdMGXhvj9Hl721c58=
=t0EP
-----END PGP SIGNATURE-----

--wwaXRVMgg64Q3zs8--