| Message ID | 1537839351-104885-1-git-send-email-yuehaibing@huawei.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [-next] drm/tegra: Use PTR_ERR_OR_ZERO in tegra_gem_create() | expand |
I'm not the maintainer, but in line with previous similar patches.. NAK: this makes the code harder to read. Thanks, Mikko On 25/09/2018 10.35, YueHaibing wrote: > Use PTR_ERR_OR_ZERO rather than if(IS_ERR(...)) + PTR_ERR > > Signed-off-by: YueHaibing <yuehaibing@huawei.com> > --- > drivers/gpu/drm/tegra/drm.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c > index e22352c..056f749 100644 > --- a/drivers/gpu/drm/tegra/drm.c > +++ b/drivers/gpu/drm/tegra/drm.c > @@ -497,10 +497,7 @@ static int tegra_gem_create(struct drm_device *drm, void *data, > > bo = tegra_bo_create_with_handle(file, drm, args->size, args->flags, > &args->handle); > - if (IS_ERR(bo)) > - return PTR_ERR(bo); > - > - return 0; > + return PTR_ERR_OR_ZERO(bo); > } > > static int tegra_gem_mmap(struct drm_device *drm, void *data, > > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
On Tue, 25 Sep 2018, Mikko Perttunen wrote: > I'm not the maintainer, but in line with previous similar patches.. > > NAK: this makes the code harder to read. If people don't like it, I wonder if it is a good thing for the function to even exist? Or at least the semantic patch that suggests this could be removed. julia > > Thanks, > Mikko > > On 25/09/2018 10.35, YueHaibing wrote: > > Use PTR_ERR_OR_ZERO rather than if(IS_ERR(...)) + PTR_ERR > > > > Signed-off-by: YueHaibing <yuehaibing@huawei.com> > > --- > > drivers/gpu/drm/tegra/drm.c | 5 +---- > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c > > index e22352c..056f749 100644 > > --- a/drivers/gpu/drm/tegra/drm.c > > +++ b/drivers/gpu/drm/tegra/drm.c > > @@ -497,10 +497,7 @@ static int tegra_gem_create(struct drm_device *drm, > > void *data, > > bo = tegra_bo_create_with_handle(file, drm, args->size, args->flags, > > &args->handle); > > - if (IS_ERR(bo)) > > - return PTR_ERR(bo); > > - > > - return 0; > > + return PTR_ERR_OR_ZERO(bo); > > } > > static int tegra_gem_mmap(struct drm_device *drm, void *data, > > > > > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > >
On 25/09/2018 16.37, Julia Lawall wrote: > > > On Tue, 25 Sep 2018, Mikko Perttunen wrote: > >> I'm not the maintainer, but in line with previous similar patches.. >> >> NAK: this makes the code harder to read. > > If people don't like it, I wonder if it is a good thing for the function > to even exist? Or at least the semantic patch that suggests this could be > removed. Good question. I think it may still have its place in some situations - e.g. if there's only one call, something like return PTR_ERR_OR_ZERO(do_something()); where this is the only potentially erroring thing in the function. In this case (and the previous ones I referred to), it's been a function with a longer series of code like variable = function(...); if (IS_ERR(variable)) return PTR_ERR(variable); and if we just change the last one it looks out of place. Although honestly, I would just write the first example in long-form as well. In the end it's a question of taste. With Tegra code we have gone for not using PTR_ERR_OR_ZERO. Cheers, Mikko > > julia > >> >> Thanks, >> Mikko >> >> On 25/09/2018 10.35, YueHaibing wrote: >>> Use PTR_ERR_OR_ZERO rather than if(IS_ERR(...)) + PTR_ERR >>> >>> Signed-off-by: YueHaibing <yuehaibing@huawei.com> >>> --- >>> drivers/gpu/drm/tegra/drm.c | 5 +---- >>> 1 file changed, 1 insertion(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c >>> index e22352c..056f749 100644 >>> --- a/drivers/gpu/drm/tegra/drm.c >>> +++ b/drivers/gpu/drm/tegra/drm.c >>> @@ -497,10 +497,7 @@ static int tegra_gem_create(struct drm_device *drm, >>> void *data, >>> bo = tegra_bo_create_with_handle(file, drm, args->size, args->flags, >>> &args->handle); >>> - if (IS_ERR(bo)) >>> - return PTR_ERR(bo); >>> - >>> - return 0; >>> + return PTR_ERR_OR_ZERO(bo); >>> } >>> static int tegra_gem_mmap(struct drm_device *drm, void *data, >>> >>> >>> >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>> >>
On Tue, Sep 25, 2018 at 04:57:30PM +0900, Mikko Perttunen wrote: > On 25/09/2018 16.37, Julia Lawall wrote: > > > > > > On Tue, 25 Sep 2018, Mikko Perttunen wrote: > > > > > I'm not the maintainer, but in line with previous similar patches.. > > > > > > NAK: this makes the code harder to read. > > > > If people don't like it, I wonder if it is a good thing for the function > > to even exist? Or at least the semantic patch that suggests this could be > > removed. > > Good question. I think it may still have its place in some situations - e.g. > if there's only one call, something like > > return PTR_ERR_OR_ZERO(do_something()); > > where this is the only potentially erroring thing in the function. > > In this case (and the previous ones I referred to), it's been a function > with a longer series of code like > > variable = function(...); > if (IS_ERR(variable)) > return PTR_ERR(variable); > > and if we just change the last one it looks out of place. Although honestly, > I would just write the first example in long-form as well. > > In the end it's a question of taste. With Tegra code we have gone for not > using PTR_ERR_OR_ZERO. This has come up recently for the Tegra PCI driver as well, and we were wondering the same thing. I know that Bjorn (PCI maintainer) and I have in the past agreed that PTR_ERR_OR_ZERO() is not something we like. For mostly the same reasons that Mikko already cited. In addition I think that PTR_ERR_OR_ZERO() makes it needlessly complicated to append code to a function. Instead of just adding a: ptr = function(...); if (IS_ERR(ptr)) return PTR_ERR(ptr); You need to split up the PTR_ERR_OR_ZERO() first and then be careful that you return the correct result, etc. I'm sure there are people that prefer PTR_ERR_OR_ZERO() over the open- coded alternative, so this is probably just something we're going to have to live with. Perhaps a good compromise would be to keep the macro around, but get rid of the semantic patch that suggests the change. At least that way we would have to go over this over and over again. Thierry
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index e22352c..056f749 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -497,10 +497,7 @@ static int tegra_gem_create(struct drm_device *drm, void *data, bo = tegra_bo_create_with_handle(file, drm, args->size, args->flags, &args->handle); - if (IS_ERR(bo)) - return PTR_ERR(bo); - - return 0; + return PTR_ERR_OR_ZERO(bo); } static int tegra_gem_mmap(struct drm_device *drm, void *data,
Use PTR_ERR_OR_ZERO rather than if(IS_ERR(...)) + PTR_ERR Signed-off-by: YueHaibing <yuehaibing@huawei.com> --- drivers/gpu/drm/tegra/drm.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)