Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
|
@serhiy-storchaka, thanks for your PR! By analyzing the history of the files in this pull request, we identified @brettcannon, @pitrou and @ericsnowcurrently to be potential reviewers. |
| with _ModuleLockManager(name): | ||
| return _find_and_load_unlocked(name, import_) | ||
| """Find and load the module.""" | ||
| _imp.acquire_lock() |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This consolidation seems a little confusing to me, but I like the idea of simplifying the C code by moving the ModuleNotFoundError generation into Python.
How about adding a dedicated _raise_for_halted_import(name, import_): function that just handles the module is None case?
| goto error; | ||
| } | ||
| else if (mod != NULL) { | ||
| if (mod != NULL && mod != Py_None) { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
With the separate helper function, we'd keep the dedicated mod == Py_None branch, but it would be simplified to just calling the new _raise_for_halted_import helper rather than constructing the exception directly in C.
| module = sys.modules[name] | ||
| if module is None: | ||
| _imp.release_lock() | ||
| message = ('import of {} halted; ' |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This would call _raise_for_halted_import(name, import_).
|
Calling Python method from the C code is not much simpler than generating an exception. The Note that the new If you prefer I can withdraw changes in I also can implement the second check in C and keep Python code unchanged. But I think that it is better to implement performancy non-critical parts in Python. |
|
@serhiy-storchaka I think it's specifically the paired negations in |
|
Do you want to swap also other branches for |
|
@serhiy-storchaka I switched my review, as coming back to it after a couple of hours, I now suspect my initial confusion was specifically due to reviewing the diff, rather than reading the updated code standalone. That realisation came mainly from comparing it to the following |
…a package. (pythonGH-2580). (cherry picked from commit b4baace)
|
GH-2598 is a backport of this pull request to the 3.6 branch. |
|
Thank you for your review @ncoghlan. |
…a package. (pythonGH-2580). (cherry picked from commit b4baace)
No description provided.