bpo-29849: fix memory leak in import_from#712
bpo-29849: fix memory leak in import_from#712
Conversation
|
@zhangyangyu, thanks for your PR! By analyzing the history of the files in this pull request, we identified @benjaminp, @serhiy-storchaka and @1st1 to be potential reviewers. |
brettcannon
left a comment
There was a problem hiding this comment.
Minor doc removal, otherwise LGTM.
| "cannot import name %R from %R (unknown location)", | ||
| name, pkgname_or_unknown | ||
| ); | ||
| /* doesn't check NULL for errmsg, PyErr_SetImportError will catch */ |
There was a problem hiding this comment.
I don't think the comment is necessary long-term.
There was a problem hiding this comment.
This comment is to make others know the error check is left out deliberately. It is not an oversight.
Do you think it's a must to remove it?
There was a problem hiding this comment.
If you want to keep it then please change it to "/* NULL check for errmsg done by PyErr_SetImportError. */".
| ); | ||
| /* doesn't check NULL for errmsg, PyErr_SetImportError will catch */ | ||
| PyErr_SetImportError(errmsg, pkgname, NULL); | ||
| } else { |
There was a problem hiding this comment.
While you're here you might as well fix the formatting for this.
| "cannot import name %R from %R (unknown location)", | ||
| name, pkgname_or_unknown | ||
| ); | ||
| /* doesn't check NULL for errmsg, PyErr_SetImportError will catch */ |
There was a problem hiding this comment.
If you want to keep it then please change it to "/* NULL check for errmsg done by PyErr_SetImportError. */".
| "cannot import name %R from %R (%S)", | ||
| name, pkgname_or_unknown, pkgpath | ||
| ); | ||
| PyErr_SetImportError(errmsg, pkgname, pkgpath); |
There was a problem hiding this comment.
Need a similar comment here if you are going to leave it in.
|
Thanks @serhiy-storchaka and @brettcannon ! |
No description provided.