#2377: Replace __import__ w/ importlib.__import__
|
Created:
1 year, 3 months ago by brett Modified:
1 year, 1 month ago Reviewers:
pjenvey, pitrou, thomas, benjamin, merwok CC:
brett.cannon, Nick Coghlan, AntoinePitrou, haypo, eric.smith, Benjamin Peterson, serwy, eric.araujo, Arfrever.FTA_GMail.Com, alex, Trundle, devnull_psf.upfronthosting.co.za, eric.snow Visibility:
Public. |
Patch Set 1 #Patch Set 2 #
Total comments: 22
Patch Set 3 #
Total comments: 71
Patch Set 4 #Patch Set 5 #
Created: 1 year, 1 month ago
MessagesTotal messages: 9
http://bugs.python.org/review/2377/diff/4262/15279 File Python/import.c (right): http://bugs.python.org/review/2377/diff/4262/15279#newcode2828 Python/import.c:2828: if (__import__ == NULL) { All of this screams for a macro, something like ADD_INTERNED in _iomodule.c http://bugs.python.org/review/2377/diff/4262/15279#newcode2895 Python/import.c:2895: level_ob = PyLong_FromLong((long)level); This doesn't seem necessary?
Sign in to reply to this message.
http://bugs.python.org/review/2377/diff/4262/15279 File Python/import.c (right): http://bugs.python.org/review/2377/diff/4262/15279#newcode2828 Python/import.c:2828: if (__import__ == NULL) { On 2012/02/25 08:02:44, pjenvey wrote: > All of this screams for a macro, something like ADD_INTERNED in _iomodule.c Done. I tried a macro before but I couldn't get it to work. Thanks for the pointer to _iomodule.c http://bugs.python.org/review/2377/diff/4262/15279#newcode2895 Python/import.c:2895: level_ob = PyLong_FromLong((long)level); On 2012/02/25 08:02:44, pjenvey wrote: > This doesn't seem necessary? Done. I had not noticed I managed to eliminate the need for it. =)
Sign in to reply to this message.
Some comments. I haven't looked at everything in detail (especially not the C code). http://bugs.python.org/review/2377/diff/4262/15250 File Lib/_importlib.py (right): http://bugs.python.org/review/2377/diff/4262/15250#newcode1 Lib/_importlib.py:1: """Core implementation of import. Is there any reason for this file, apart from aesthetics? I liked importlib/_bootstrap.py, myself. http://bugs.python.org/review/2377/diff/4262/15270 File Lib/test/test_frozen.py (left): http://bugs.python.org/review/2377/diff/4262/15270#oldcode16 Lib/test/test_frozen.py:16: self.assertEqual(stdout.getvalue(), 'Hello world!\n') Any reason why these tests were removed? http://bugs.python.org/review/2377/diff/4262/15273 File Makefile.pre.in (right): http://bugs.python.org/review/2377/diff/4262/15273#newcode764 Makefile.pre.in:764: $(srcdir)/Python/importlib.h \ The Makefile should have the necessary build steps to regenerate importlib.h when it's outdated. http://bugs.python.org/review/2377/diff/4262/15275 File Modules/_io/textio.c (right): http://bugs.python.org/review/2377/diff/4262/15275#newcode882 Modules/_io/textio.c:882: "unable to import the os module"); I think on-demand importing of os is not good. Since device_encoding is in posixmodule.c, the _os module should be imported at startup instead. Or, better, the C function for device_encoding could be put in the core (Python/fileutils.c?). http://bugs.python.org/review/2377/diff/4262/15277 File Python/bltinmodule.c (right): http://bugs.python.org/review/2377/diff/4262/15277#newcode190 Python/bltinmodule.c:190: int level = 0; That's probably not important, but out of curiousity, why the change? http://bugs.python.org/review/2377/diff/4262/15279 File Python/import.c (right): http://bugs.python.org/review/2377/diff/4262/15279#newcode283 Python/import.c:283: PyErr_Print(); Strange indentation here. http://bugs.python.org/review/2377/diff/4262/15280 File Python/importlib.h (right): http://bugs.python.org/review/2377/diff/4262/15280#newcode1 Python/importlib.h:1: unsigned char M__importlib[] = { You should add a header (comment) explaining that's it's machine-generated and how. http://bugs.python.org/review/2377/diff/4262/15282 File Python/pythonrun.c (right): http://bugs.python.org/review/2377/diff/4262/15282#newcode227 Python/pythonrun.c:227: Py_FatalError("Py_Initialize: importlib install failed"); You probably want to print the traceback for easier debugging (assuming we have the rescue stderr at this point; I suppose we do?). http://bugs.python.org/review/2377/diff/4262/15283 File Tools/freeze/freeze_importlib.py (right): http://bugs.python.org/review/2377/diff/4262/15283#newcode13 Tools/freeze/freeze_importlib.py:13: """ I guess this file needs cleaning up. A couple of comments (or a docstring) woule be good too.
Sign in to reply to this message.
http://bugs.python.org/review/2377/diff/4262/15250 File Lib/_importlib.py (right): http://bugs.python.org/review/2377/diff/4262/15250#newcode1 Lib/_importlib.py:1: """Core implementation of import. There was when I was bootstrapping using import.c and I wanted to avoid importlib.__init__. That's over now, though, so I moved the file back. http://bugs.python.org/review/2377/diff/4262/15270 File Lib/test/test_frozen.py (left): http://bugs.python.org/review/2377/diff/4262/15270#oldcode16 Lib/test/test_frozen.py:16: self.assertEqual(stdout.getvalue(), 'Hello world!\n') On 2012/02/27 21:13:35, AntoinePitrou wrote: > Any reason why these tests were removed? > Nope, probably a bad merge at some point. I copied test_frozen.py from default and then fixed the test to pass again. http://bugs.python.org/review/2377/diff/4262/15273 File Makefile.pre.in (right): http://bugs.python.org/review/2377/diff/4262/15273#newcode764 Makefile.pre.in:764: $(srcdir)/Python/importlib.h \ On 2012/02/27 21:13:35, AntoinePitrou wrote: > The Makefile should have the necessary build steps to regenerate importlib.h > when it's outdated. > Done. http://bugs.python.org/review/2377/diff/4262/15275 File Modules/_io/textio.c (right): http://bugs.python.org/review/2377/diff/4262/15275#newcode882 Modules/_io/textio.c:882: "unable to import the os module"); On 2012/02/27 21:13:35, AntoinePitrou wrote: > I think on-demand importing of os is not good. Since device_encoding is in > posixmodule.c, the _os module should be imported at startup instead. > Or, better, the C function for device_encoding could be put in the core > (Python/fileutils.c?). > Done. http://bugs.python.org/review/2377/diff/4262/15277 File Python/bltinmodule.c (right): http://bugs.python.org/review/2377/diff/4262/15277#newcode190 Python/bltinmodule.c:190: int level = 0; On 2012/02/27 21:13:35, AntoinePitrou wrote: > That's probably not important, but out of curiousity, why the change? I'm not supporting a level of -1 as it makes no bloody sense. PEP 328 killed the concept of a level of -1 and yet it continues to live on in import.c with totally murking semantics. If you want to try a relative import after an absolute one, then catch the ImportError and try again with a level of -1. And it just gets worse that import.c will take any negative level and treat them all the same. http://bugs.python.org/review/2377/diff/4262/15279 File Python/import.c (right): http://bugs.python.org/review/2377/diff/4262/15279#newcode283 Python/import.c:283: PyErr_Print(); On 2012/02/27 21:13:35, AntoinePitrou wrote: > Strange indentation here. > Done. http://bugs.python.org/review/2377/diff/4262/15280 File Python/importlib.h (right): http://bugs.python.org/review/2377/diff/4262/15280#newcode1 Python/importlib.h:1: unsigned char M__importlib[] = { On 2012/02/27 21:13:35, AntoinePitrou wrote: > You should add a header (comment) explaining that's it's machine-generated and > how. > Done. http://bugs.python.org/review/2377/diff/4262/15282 File Python/pythonrun.c (right): http://bugs.python.org/review/2377/diff/4262/15282#newcode227 Python/pythonrun.c:227: Py_FatalError("Py_Initialize: importlib install failed"); On 2012/02/27 21:13:35, AntoinePitrou wrote: > You probably want to print the traceback for easier debugging (assuming we have > the rescue stderr at this point; I suppose we do?). > You mean _Py_DumpTraceback() w/ stderr manually passed? http://bugs.python.org/review/2377/diff/4262/15283 File Tools/freeze/freeze_importlib.py (right): http://bugs.python.org/review/2377/diff/4262/15283#newcode13 Tools/freeze/freeze_importlib.py:13: """ On 2012/02/27 21:13:35, AntoinePitrou wrote: > I guess this file needs cleaning up. A couple of comments (or a docstring) woule > be good too. Completely moved over to Python/freeze_importlib.py
Sign in to reply to this message.
LGTM. Only commenting on the easily fixed or obvious issues, we can fix more things after it actually lands. http://bugs.python.org/review/2377/diff/4307/15436 File Lib/importlib/machinery.py (left): http://bugs.python.org/review/2377/diff/4307/15436#oldcode2 Lib/importlib/machinery.py:2: gratuitous change? http://bugs.python.org/review/2377/diff/4307/15444 File Lib/importlib/test/util.py (left): http://bugs.python.org/review/2377/diff/4307/15444#oldcode38 Lib/importlib/test/util.py:38: "cannot uncache {0} as it will break _importlib".format(name)) I suggest removing the 'as it will break importlib' part altogether. http://bugs.python.org/review/2377/diff/4307/15445 File Lib/os.py (right): http://bugs.python.org/review/2377/diff/4307/15445#newcode45 Lib/os.py:45: # Any introduction of a new "_os" module and/or path separator requires "_os" is a little vague. How about "Any new dependencies of the os module and/or changes in path separator requires updating importlib as well"? http://bugs.python.org/review/2377/diff/4307/15447 File Lib/test/test_frozen.py (right): http://bugs.python.org/review/2377/diff/4307/15447#newcode15 Lib/test/test_frozen.py:15: self.assertEqual(len(dir(__hello__)), 8, dir(__hello__)) This should probably document why you expect 8 attributes, or (better yet) explicitly compare a set of attribute names. http://bugs.python.org/review/2377/diff/4307/15453 File Makefile.pre.in (right): http://bugs.python.org/review/2377/diff/4307/15453#newcode555 Makefile.pre.in:555: ./$(BUILDPYTHON) Python/freeze_importlib.py \ I suspect this will break obj-directory builds again. Have you tried it like that? http://bugs.python.org/review/2377/diff/4307/15456 File Python/freeze_importlib.py (right): http://bugs.python.org/review/2377/diff/4307/15456#newcode16 Python/freeze_importlib.py:16: lines.append('unsigned char M__importlib[] = {') This should have a _Py_ prefix. http://bugs.python.org/review/2377/diff/4307/15456#newcode21 Python/freeze_importlib.py:21: for c in bytes(data[i:i+16]): This bytes() call seems unnecessary considering you are running this (and you have to run this) with the built python. It will always be Python 3. http://bugs.python.org/review/2377/diff/4307/15456#newcode34 Python/freeze_importlib.py:34: raise RuntimeError('need to specify input and output file paths') This seems rather weird use of RuntimeError. It's not an error in the runtime, just in the caller. http://bugs.python.org/review/2377/diff/4307/15457 File Python/frozen.c (right): http://bugs.python.org/review/2377/diff/4307/15457#newcode35 Python/frozen.c:35: {"__hello__", M___hello__, SIZE}, These should also be fixed to use _Py or Py as a prefix, as while we're at it :) http://bugs.python.org/review/2377/diff/4307/15458 File Python/import.c (right): http://bugs.python.org/review/2377/diff/4307/15458#newcode272 Python/import.c:272: goto error; This change appears to be leaking a reference. http://bugs.python.org/review/2377/diff/4307/15458#newcode2804 Python/import.c:2804: if (!(name = PyUnicode_InternFromString(# name))) \ Predominate style in the codebase is to use no space between '#' and 'name'. http://bugs.python.org/review/2377/diff/4307/15458#newcode2843 Python/import.c:2843: Py_FatalError("can't create an empty dict"); Why are these fatal errors but the failures to intern, above, not? http://bugs.python.org/review/2377/diff/4307/15458#newcode2893 Python/import.c:2893: PyObject *partition = PyUnicode_RPartition(package, dot); Need to check the returnvalue of PyUnicode_RPartition() and/or the type of package (which you do for the __package__ case but not the __name__ case.) http://bugs.python.org/review/2377/diff/4307/15458#newcode3039 Python/import.c:3039: Try to move sys.modules check as high up as possible Get rid of this comment? Or do you still want to duplicate the sys.module checks in C as well?
Sign in to reply to this message.
Mostly, I just hope you've run with refleak detection enabled. :) http://bugs.python.org/review/2377/diff/4307/15432 File FAILING (right): http://bugs.python.org/review/2377/diff/4307/15432#newcode1 FAILING:1: # Code to detect import failure no longer works (relied on stack depth). The whole file should of course die before it is integrated. http://bugs.python.org/review/2377/diff/4307/15434 File Include/pystate.h (right): http://bugs.python.org/review/2377/diff/4307/15434#newcode36 Include/pystate.h:36: PyObject *importlib; Maybe this should go up with the other import related things like sysdict, builtins, modules_reloading... http://bugs.python.org/review/2377/diff/4307/15441 File Lib/importlib/test/source/test_finder.py (right): http://bugs.python.org/review/2377/diff/4307/15441#newcode4 Lib/importlib/test/source/test_finder.py:4: from importlib import _bootstrap Why are half of these files importing import._bootstrap and the other half just _bootstrap? http://bugs.python.org/review/2377/diff/4307/15446 File Lib/site.py (right): http://bugs.python.org/review/2377/diff/4307/15446#newcode85 Lib/site.py:85: if m.__loader__.__module__ != '_frozen_importlib': This condition can be folded into the if above with a getattr. Also, do you want to require __loader__ to have a __module__? http://bugs.python.org/review/2377/diff/4307/15456 File Python/freeze_importlib.py (right): http://bugs.python.org/review/2377/diff/4307/15456#newcode1 Python/freeze_importlib.py:1: #! /usr/bin/env python Can we put this in Tools/scripts? http://bugs.python.org/review/2377/diff/4307/15456#newcode10 Python/freeze_importlib.py:10: with open(input_path, 'r') as input_file: No encoding? :) http://bugs.python.org/review/2377/diff/4307/15458 File Python/import.c (right): http://bugs.python.org/review/2377/diff/4307/15458#newcode2830 Python/import.c:2830: ADD_INTERNED(__import__); You should use the _Py_identifier API for all these. http://bugs.python.org/review/2377/diff/4307/15458#newcode2855 Python/import.c:2855: globals = empty_dict; Instead of using these static dicts, how about just calling PyDict_New() if you need an empty dict every time? You could even just check for NULL in the places that are using the globals and fromlist objects. It's always nicer not to have immortal static stuff sitting around. http://bugs.python.org/review/2377/diff/4307/15458#newcode2887 Python/import.c:2887: if (package == NULL) { If __name__ is simply not in globals, shouldn't we set an error? http://bugs.python.org/review/2377/diff/4307/15458#newcode2892 Python/import.c:2892: if (PyDict_GetItem(globals, __path__) == NULL) { Why do we use PyDict_GetItemWithError some of the time and PyDict_GetItem other times? http://bugs.python.org/review/2377/diff/4307/15458#newcode2909 Python/import.c:2909: if (PyObject_Not(name)) { We know name is name is unicode so how about using PyUnicode_GET_LENGTH? (If you do that, make sure to call PyUnicode_READY.) http://bugs.python.org/review/2377/diff/4307/15458#newcode2935 Python/import.c:2935: if (PyObject_IsTrue(name)) { Once again, PyUnicode_GET_LENGTH to the rescue. Alternatively check if last_dot == 0. http://bugs.python.org/review/2377/diff/4307/15458#newcode2936 Python/import.c:2936: PyObject *seq = PyTuple_New(2); PyTuple_Pack instead? http://bugs.python.org/review/2377/diff/4307/15458#newcode2960 Python/import.c:2960: if (PyDict_Check(globals)) { Haven't you already been treating it like a dictionary? http://bugs.python.org/review/2377/diff/4307/15458#newcode2988 Python/import.c:2988: if (PyObject_Not(fromlist)) { PyList_GET_SIZE? I suppose you would have to force it to be a list. http://bugs.python.org/review/2377/diff/4307/15458#newcode2989 Python/import.c:2989: if (level == 0 || PyObject_IsTrue(name)) { PyUnicode_GET_LENGTH again. http://bugs.python.org/review/2377/diff/4307/15458#newcode3012 Python/import.c:3012: final_mod = PyDict_GetItem(interp->modules, to_return); Don't we need to INCREF final_mod somewhere now? http://bugs.python.org/review/2377/diff/4307/15461 File Python/pythonrun.c (right): http://bugs.python.org/review/2377/diff/4307/15461#newcode709 Python/pythonrun.c:709: import_init(interp, sysmod); We don't init zipimport here?
Sign in to reply to this message.
http://bugs.python.org/review/2377/diff/4307/15456 File Python/freeze_importlib.py (right): http://bugs.python.org/review/2377/diff/4307/15456#newcode1 Python/freeze_importlib.py:1: #! /usr/bin/env python > Can we put this in Tools/scripts? Seconded; some modules are regenerated by themselves (e.g. token), other by scripts in Tools (e.g. unicodedata), and only one Python script is found in the Python directory (makeopcodetargets.py). http://bugs.python.org/review/2377/diff/4307/15456#newcode34 Python/freeze_importlib.py:34: raise RuntimeError('need to specify input and output file paths') > This seems rather weird use of RuntimeError. A suggestion: sys.exit('need to specify etc.')
Sign in to reply to this message.
http://bugs.python.org/review/2377/diff/4307/15458 File Python/import.c (right): http://bugs.python.org/review/2377/diff/4307/15458#newcode283 Python/import.c:283: PyErr_Print(); Bogus indentation here. http://bugs.python.org/review/2377/diff/4307/15461 File Python/pythonrun.c (right): http://bugs.python.org/review/2377/diff/4307/15461#newcode227 Python/pythonrun.c:227: Py_FatalError("Py_Initialize: importlib install failed"); Uh, please print the actual error before the fatal error. It will make debugging much easier.
Sign in to reply to this message.
Still have three failures left, but they do not stem from any changes made here. http://bugs.python.org/review/2377/diff/4307/15432 File FAILING (right): http://bugs.python.org/review/2377/diff/4307/15432#newcode1 FAILING:1: # Code to detect import failure no longer works (relied on stack depth). On 2012/03/15 17:18:34, Benjamin Peterson wrote: > The whole file should of course die before it is integrated. Yes., of course. I will probably just generate a massive patch against default and commit that since the revision history for this branch is not critical (and full of branch merges anyway). http://bugs.python.org/review/2377/diff/4307/15434 File Include/pystate.h (right): http://bugs.python.org/review/2377/diff/4307/15434#newcode36 Include/pystate.h:36: PyObject *importlib; On 2012/03/15 17:18:34, Benjamin Peterson wrote: > Maybe this should go up with the other import related things like sysdict, > builtins, modules_reloading... Done. http://bugs.python.org/review/2377/diff/4307/15436 File Lib/importlib/machinery.py (left): http://bugs.python.org/review/2377/diff/4307/15436#oldcode2 Lib/importlib/machinery.py:2: On 2012/03/13 01:00:03, twouters wrote: > gratuitous change? It's an artifact from when I renamed importlib._bootstrap to _importlib for a time. I inserted the newline again. http://bugs.python.org/review/2377/diff/4307/15441 File Lib/importlib/test/source/test_finder.py (right): http://bugs.python.org/review/2377/diff/4307/15441#newcode4 Lib/importlib/test/source/test_finder.py:4: from importlib import _bootstrap On 2012/03/15 17:18:34, Benjamin Peterson wrote: > Why are half of these files importing import._bootstrap and the other half just > _bootstrap? Beats me. =) This code has grown somewhat organically, especially the test suite. This can be unified, but it should be done in trunk and probably part of moving the test suite out of Lib/importlib/test and into Lib/test/importlib. http://bugs.python.org/review/2377/diff/4307/15444 File Lib/importlib/test/util.py (left): http://bugs.python.org/review/2377/diff/4307/15444#oldcode38 Lib/importlib/test/util.py:38: "cannot uncache {0} as it will break _importlib".format(name)) On 2012/03/13 01:00:03, twouters wrote: > I suggest removing the 'as it will break importlib' part altogether. Done. http://bugs.python.org/review/2377/diff/4307/15445 File Lib/os.py (right): http://bugs.python.org/review/2377/diff/4307/15445#newcode45 Lib/os.py:45: # Any introduction of a new "_os" module and/or path separator requires On 2012/03/13 01:00:03, twouters wrote: > "_os" is a little vague. How about "Any new dependencies of the os module and/or > changes in path separator requires updating importlib as well"? Done. http://bugs.python.org/review/2377/diff/4307/15446 File Lib/site.py (right): http://bugs.python.org/review/2377/diff/4307/15446#newcode85 Lib/site.py:85: if m.__loader__.__module__ != '_frozen_importlib': On 2012/03/15 17:18:34, Benjamin Peterson wrote: > This condition can be folded into the if above with a getattr. Also, do you want > to require __loader__ to have a __module__? Done. http://bugs.python.org/review/2377/diff/4307/15447 File Lib/test/test_frozen.py (right): http://bugs.python.org/review/2377/diff/4307/15447#newcode15 Lib/test/test_frozen.py:15: self.assertEqual(len(dir(__hello__)), 8, dir(__hello__)) On 2012/03/13 01:00:03, twouters wrote: > This should probably document why you expect 8 attributes, or (better yet) > explicitly compare a set of attribute names. Done. http://bugs.python.org/review/2377/diff/4307/15453 File Makefile.pre.in (right): http://bugs.python.org/review/2377/diff/4307/15453#newcode555 Makefile.pre.in:555: ./$(BUILDPYTHON) Python/freeze_importlib.py \ On 2012/03/13 01:00:03, twouters wrote: > I suspect this will break obj-directory builds again. Have you tried it like > that? I had not. Fixed and tested by running ./configure from Objects/. http://bugs.python.org/review/2377/diff/4307/15456 File Python/freeze_importlib.py (right): http://bugs.python.org/review/2377/diff/4307/15456#newcode1 Python/freeze_importlib.py:1: #! /usr/bin/env python On 2012/03/15 23:03:53, eric.araujo wrote: > > Can we put this in Tools/scripts? > Seconded; some modules are regenerated by themselves (e.g. token), other by > scripts in Tools (e.g. unicodedata), and only one Python script is found in the > Python directory (makeopcodetargets.py). So I had it there but I think Antoine asked me to move it, so I will wait for a more solid answer from people on where to keep it. http://bugs.python.org/review/2377/diff/4307/15456#newcode10 Python/freeze_importlib.py:10: with open(input_path, 'r') as input_file: On 2012/03/15 17:18:34, Benjamin Peterson wrote: > No encoding? :) Done. http://bugs.python.org/review/2377/diff/4307/15456#newcode16 Python/freeze_importlib.py:16: lines.append('unsigned char M__importlib[] = {') On 2012/03/13 01:00:03, twouters wrote: > This should have a _Py_ prefix. Done. http://bugs.python.org/review/2377/diff/4307/15456#newcode21 Python/freeze_importlib.py:21: for c in bytes(data[i:i+16]): On 2012/03/13 01:00:03, twouters wrote: > This bytes() call seems unnecessary considering you are running this (and you > have to run this) with the built python. It will always be Python 3. Done. http://bugs.python.org/review/2377/diff/4307/15456#newcode34 Python/freeze_importlib.py:34: raise RuntimeError('need to specify input and output file paths') On 2012/03/15 23:03:53, eric.araujo wrote: > > This seems rather weird use of RuntimeError. > A suggestion: sys.exit('need to specify etc.') Done. http://bugs.python.org/review/2377/diff/4307/15457 File Python/frozen.c (right): http://bugs.python.org/review/2377/diff/4307/15457#newcode35 Python/frozen.c:35: {"__hello__", M___hello__, SIZE}, On 2012/03/13 01:00:03, twouters wrote: > These should also be fixed to use _Py or Py as a prefix, as while we're at it :) Done. http://bugs.python.org/review/2377/diff/4307/15458 File Python/import.c (right): http://bugs.python.org/review/2377/diff/4307/15458#newcode272 Python/import.c:272: goto error; On 2012/03/13 01:00:03, twouters wrote: > This change appears to be leaking a reference. Done. http://bugs.python.org/review/2377/diff/4307/15458#newcode283 Python/import.c:283: PyErr_Print(); On 2012/03/16 23:55:21, AntoinePitrou wrote: > Bogus indentation here. > Done. http://bugs.python.org/review/2377/diff/4307/15458#newcode2804 Python/import.c:2804: if (!(name = PyUnicode_InternFromString(# name))) \ On 2012/03/13 01:00:03, twouters wrote: > Predominate style in the codebase is to use no space between '#' and 'name'. Done. http://bugs.python.org/review/2377/diff/4307/15458#newcode2830 Python/import.c:2830: ADD_INTERNED(__import__); On 2012/03/15 17:18:34, Benjamin Peterson wrote: > You should use the _Py_identifier API for all these. Done. http://bugs.python.org/review/2377/diff/4307/15458#newcode2843 Python/import.c:2843: Py_FatalError("can't create an empty dict"); On 2012/03/13 01:00:03, twouters wrote: > Why are these fatal errors but the failures to intern, above, not? No good reason. But I will move all of this over to _Py_IDENTIFIER usage so that will make this a moot point. http://bugs.python.org/review/2377/diff/4307/15458#newcode2855 Python/import.c:2855: globals = empty_dict; On 2012/03/15 17:18:34, Benjamin Peterson wrote: > Instead of using these static dicts, how about just calling PyDict_New() if you > need an empty dict every time? You could even just check for NULL in the places > that are using the globals and fromlist objects. It's always nicer not to have > immortal static stuff sitting around. Done. I originally did all of this static stuff to avoid any perf overhead since importlib's bootstrap acceptance depended on it. http://bugs.python.org/review/2377/diff/4307/15458#newcode2887 Python/import.c:2887: if (package == NULL) { On 2012/03/15 17:18:34, Benjamin Peterson wrote: > If __name__ is simply not in globals, shouldn't we set an error? That's what PyDict_GetItemWithError() does. http://bugs.python.org/review/2377/diff/4307/15458#newcode2892 Python/import.c:2892: if (PyDict_GetItem(globals, __path__) == NULL) { On 2012/03/15 17:18:34, Benjamin Peterson wrote: > Why do we use PyDict_GetItemWithError some of the time and PyDict_GetItem other > times? Sometimes lacking a key is an error, sometimes it's not. =) http://bugs.python.org/review/2377/diff/4307/15458#newcode2893 Python/import.c:2893: PyObject *partition = PyUnicode_RPartition(package, dot); On 2012/03/13 01:00:03, twouters wrote: > Need to check the returnvalue of PyUnicode_RPartition() and/or the type of > package (which you do for the __package__ case but not the __name__ case.) Done. http://bugs.python.org/review/2377/diff/4307/15458#newcode2909 Python/import.c:2909: if (PyObject_Not(name)) { On 2012/03/15 17:18:34, Benjamin Peterson wrote: > We know name is name is unicode so how about using PyUnicode_GET_LENGTH? (If you > do that, make sure to call PyUnicode_READY.) Done. http://bugs.python.org/review/2377/diff/4307/15458#newcode2935 Python/import.c:2935: if (PyObject_IsTrue(name)) { On 2012/03/15 17:18:34, Benjamin Peterson wrote: > Once again, PyUnicode_GET_LENGTH to the rescue. Alternatively check if last_dot > == 0. Done. http://bugs.python.org/review/2377/diff/4307/15458#newcode2935 Python/import.c:2935: if (PyObject_IsTrue(name)) { On 2012/03/15 17:18:34, Benjamin Peterson wrote: > Once again, PyUnicode_GET_LENGTH to the rescue. Alternatively check if last_dot > == 0. Done. http://bugs.python.org/review/2377/diff/4307/15458#newcode2936 Python/import.c:2936: PyObject *seq = PyTuple_New(2); On 2012/03/15 17:18:34, Benjamin Peterson wrote: > PyTuple_Pack instead? Done. http://bugs.python.org/review/2377/diff/4307/15458#newcode2960 Python/import.c:2960: if (PyDict_Check(globals)) { On 2012/03/15 17:18:34, Benjamin Peterson wrote: > Haven't you already been treating it like a dictionary? Only when level > 0. And I have to let non-dicts through; lib2to3 does a very naughty import of setting globals to []. =) http://bugs.python.org/review/2377/diff/4307/15458#newcode2988 Python/import.c:2988: if (PyObject_Not(fromlist)) { On 2012/03/15 17:18:34, Benjamin Peterson wrote: > PyList_GET_SIZE? I suppose you would have to force it to be a list. Exactly. http://bugs.python.org/review/2377/diff/4307/15458#newcode2989 Python/import.c:2989: if (level == 0 || PyObject_IsTrue(name)) { On 2012/03/15 17:18:34, Benjamin Peterson wrote: > PyUnicode_GET_LENGTH again. Done. http://bugs.python.org/review/2377/diff/4307/15458#newcode3012 Python/import.c:3012: final_mod = PyDict_GetItem(interp->modules, to_return); On 2012/03/15 17:18:34, Benjamin Peterson wrote: > Don't we need to INCREF final_mod somewhere now? Done. http://bugs.python.org/review/2377/diff/4307/15458#newcode3039 Python/import.c:3039: Try to move sys.modules check as high up as possible On 2012/03/13 01:00:03, twouters wrote: > Get rid of this comment? Or do you still want to duplicate the sys.module checks > in C as well? Removed. http://bugs.python.org/review/2377/diff/4307/15461 File Python/pythonrun.c (right): http://bugs.python.org/review/2377/diff/4307/15461#newcode227 Python/pythonrun.c:227: Py_FatalError("Py_Initialize: importlib install failed"); On 2012/03/16 23:55:21, AntoinePitrou wrote: > Uh, please print the actual error before the fatal error. It will make debugging > much easier. I'm hoping PyErr_Print() will use some fallback stderr since the I/O system is not up and running at this point. http://bugs.python.org/review/2377/diff/4307/15461#newcode709 Python/pythonrun.c:709: import_init(interp, sysmod); On 2012/03/15 17:18:34, Benjamin Peterson wrote: > We don't init zipimport here? You might be mislead by the diff as there is a call to _PyImportZip_Init() in the actual code. But if you mean why don't I call _PyImportZip_Init(), I will move it into import_init().
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
