Skip to content

Return 0 or -1 from ImagingTransform() - #10075

Open
radarhere wants to merge 1 commit into
python-pillow:mainfrom
radarhere:return
Open

radarhere wants to merge 1 commit into
python-pillow:mainfrom
radarhere:return

Conversation

@radarhere

Copy link
Copy Markdown
Member

Pillow/src/_imaging.c

Lines 2170 to 2180 in 38c17ed

imOut = ImagingTransform(
self->image, imagep->image, method, x0, y0, x1, y1, a, filter, fill
);
free(a);
if (!imOut) {
return NULL;
}
Py_RETURN_NONE;

This looks odd to me, because imOut is checked for an error, but even if there is no error, it is discarded?

The reason is that ImagingTransform() is actually passing back the first argument as imOut on success, and NULL on failure.

I think it would be clearer to return 0 on success, and -1 on failure.

@akx akx left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond the inline comments, I guess it would be nice to also add C doc comments explaining the retval and who's responsible for freeing the image on error?

Comment thread src/_imaging.c
imOut = ImagingNewDirty(imIn->mode, xsize, ysize);

imOut = ImagingTransform(
ImagingTransform(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be an error check here?

Comment thread src/libImaging/Geometry.c
@@ -883,7 +886,8 @@ ImagingScaleAffine(
xintab = (int *)calloc(out_xsize, sizeof(int));
if (!xintab) {
ImagingDelete(imOut);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So... whose responsibility it is to delete imOut? 🤔 The above error returns do not delete it, this does?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants