> On May 2, 2013, 8:22 a.m., Nathan Binkert wrote: > > Looks good. I'm curious, were there any bugs caused by this?
I don't think it causes any harm at the moment. The reason I prepared the patch in the first place was that I was debugging some other loader related issue (I think it turned out to be SWIG that was broken) and I wanted to eliminated potential problems. > On May 2, 2013, 8:22 a.m., Nathan Binkert wrote: > > src/python/importer.py, line 57 > > <http://reviews.gem5.org/r/1857/diff/1/?file=35349#file35349line57> > > > > I'd say don't call imp.new_module unless you actually fail to find the > > module (Python evaluates all arguments to a function before the function is > > called, so the new module is created with setdefault): > > > > try: > > mod = sys.modules[fullname] > > except KeyError: > > mod = imp.new_module(fullname) > > sys.modules[fullname] = mod I'll change that before I commit. I think the current code is from the example pattern in PEP302. I was under the impression that this is behavior is completely harmless anyway since the new object will be freed by the reference counting GC right away. - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1857/#review4313 ----------------------------------------------------------- On May 2, 2013, 6:59 a.m., Andreas Sandberg wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1857/ > ----------------------------------------------------------- > > (Updated May 2, 2013, 6:59 a.m.) > > > Review request for Default. > > > Description > ------- > > Changeset 9688:349fa0e3e253 > --------------------------- > base: Make the Python module loader PEP302 compliant > > The custom Python loader doesn't comply with PEP302 for two reasons: > > * Previously, we would overwrite old modules on name > conflicts. PEP302 explicitly states that: "If there is an existing > module object named 'fullname' in sys.modules, the loader must use > that existing module". > > * The "__package__" attribute wasn't set. PEP302: "The __package__ > attribute must be set." > > This changeset addresses both of these issues. > > > Diffs > ----- > > src/python/importer.py 00dca8a9b560 > > Diff: http://reviews.gem5.org/r/1857/diff/ > > > Testing > ------- > > Quick regressions pass (with the exception of 02.insttest and > 40.m5threads-test-atomic where I can't find the binaries) for all supported > architectures. > > > Thanks, > > Andreas Sandberg > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
