| msg51714 - (view) |
Author: Neal Norwitz (nnorwitz) *  |
Date: 2007-01-09 06:29 |
| Re-implement the warnings module in C for speed and to reduce start up time. I don't remember the exact state of this patch. I'm sure it needs cleanup. IIRC the only thing missing feature-wise was processing command line arguments. Though I'm not entirely sure. It's been a while since I did it. I think I may have not used as many goto's in the code. I'm also thinking I didn't like it as the error handling was too complex. This definitely needs review. If anyone wants to finish this off, go for it. I'll probably return to it, but it won't be for a few weeks at the earliest. It would probably be good to make comments to remind me of what needs to be done. The new file should be Python/_warnings.c. I couldn't decide whether to put it under Python/ or Modules/. It seems some builtin modules are in both places. Maybe we should determine where the appropriate place is and move them all there. I couldn't figure out how to get svn to do a diff of a file that wasn't checked in. I think I filtered out all the unrelated changes. |
|
|
| msg51715 - (view) |
Author: Neal Norwitz (nnorwitz) * |
Date: 2007-01-09 06:30 |
| File Added: _warnings.c |
|
|
| msg56283 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2007-10-09 05:43 |
| I have uploaded my touched up version of _warnings.c . I also have a new diff since the old one did not apply cleanly (test_warnings didn't get patched nor did PCbuild8). It also lacked the changes needed to Modules/config.c . It is still not complete, though. Some things are missing from _warnings.c and test_warnings is completely failing. But I figured I should get something uploaded so that other people can help if they want. |
|
|
| msg56288 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2007-10-09 17:55 |
| I just tried Neal's version of _warnings.c and it has the same failures, so they are not my fault. =) |
|
|
| msg56291 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2007-10-09 19:53 |
| I figured out why the tests are all failing; the C code does not call back into the Python 'warnings' wrapper. For instance, warn_explicit in the C code does not ever try to use a user-provided showwarning() function. This causes the tests to fail as they rely on this functionality. There is also the issue of the filters and once_registry also only be referenced in the C code and not in the Python code. So while the list and dict, respectively, are assigned in warnings.py from _warnings.c, the C code never checks to see if the attributes in the Python code were changed. This is an issue as a lot of times code does:: warnings.filter = [] ... code ... warnings.filter = original_filter That will not work with how it is implemented now as the C code only works off of the object it created and never one that a user may have provided. Could a descriptor be written so that when the Python code has the filter, once_registry, or showwarnings set it actually gets set in the C code? That way the C code can continue to be fully independent of the Python code and not have to import it unless a specific change was attempted upon 'warnings'? |
|
|
| msg56409 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2007-10-14 04:55 |
| So the descriptor idea didn't work. Another idea is to have the C code that relies on attributes on warnings that are allowed to change have an initial check for warnings, and if that fails to fall back on C code. That way the module can still be completely self-sufficient while still allowing users to change values from Python code. For instance, take warnings.filters. Initially it can be set to _warnings.filters. But in the C code, where access to the filters list is needed, a check is first done to see if the warnings module has been imported. If so, check for a filters attribute. If it exists, replace the internal list with that one and continue on. If the module is not there, however, use the list stored statically in the module without change. Same idea of the once registry and a similar idea for showwarnings(). |
|
|
| msg56509 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2007-10-17 00:35 |
| Attached is a new version of _warnings.c that checks to see if 'warnings' has been imported, and if so, uses the attributes from that module for onceregistry and 'filters'. I did it in such a way so that 'warnings' is in no way required nor imported through checking. If Neal does a code review and OKs the approach then it can also be implemented for showwarning() which should make testing test_warnings possible. =) Then we can start testing that changes to the module attributes actually affect things properly. |
|
|
| msg56517 - (view) |
Author: Neal Norwitz (nnorwitz) * |
Date: 2007-10-18 03:53 |
| I think this is good enough for now. The approach will probably stand even if the details change. Go for it! |
|
|
| msg56545 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2007-10-18 21:41 |
| Regression test suite now passes. =) Had to add the support for when warnings.showwarning() is set and a bug in PyErr_WarnExplicit() where a NULL value for the module name was not being allowed. Couple things still left to implement. One is the second output line in show_warning(). Don't notice this in unit tests as it imports warnings and thus uses the Python implementation that works properly. Also need to implement warn_explicit(). Lastly, -W arguments need to be handled. In terms of differing semantics, file paths are different. Old way did absolute paths. It also handled what the file path should be when executing a warning from the interpreter differently. In terms of testing, some _warnings-specific tests are needed. As mentioned above, the differences between _warnings.show_warning() and warnings.showwarning() are not being picked up. This shows how tests for setting of 'filters', 'onceregistry', and 'showwarning' needs to be tested. Assigning to Neal to see if there is anything I missed in terms of todos and see if he wants to take any of them on. =) |
|
|
| msg56983 - (view) |
Author: Neal Norwitz (nnorwitz) * |
Date: 2007-10-31 04:22 |
| I think Brett summarized the issues well. I can't think of anything else that seems to need doing. |
|
|
| msg57627 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2007-11-18 23:45 |
| This version of test_warnings has tests for _warnings.c. It currently is failing as the second line of output for warnings has not been implemented yet for _warnings.c. But all the specified tests in my last comment have now been implemented. |
|
|
| msg57670 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2007-11-19 22:35 |
| Attached is a diff against the trunk (including _warnings.c) that adds the second line of output. That leaves warn_explicit() and handling -W arguments on the TODO list. I really don't want to do the -W arguments, so anyone who wants to take that on, feel free. =) |
|
|
| msg57767 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2007-11-22 20:26 |
| Implementing warn_explicit() is going to be troublesome with the new module_globals argument. It requires not only to go through the hassle of looking for a loader and calling get_source(), but it will most likely require reworking the traceback function introduced to print out the second line of output (it doesn't look like it directly supports loaders, but I have not tried it personally). |
|
|
| msg57968 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2007-11-30 07:57 |
| I see two ways of implementing the fetching of a source code line from __loader__.get_source(). One is to do it in Python. We have a function provided that can suppress the second line of output from a warning and just handle it in Python code. That has the requirement that Python code be available to import, but if you are using Lib/warnings.py instead of Python/_warnings.c that is pretty much guaranteed. The other option is to rely on the fact that get_source() is supposed to use universal newlines. Then we can find the index of the x and x-1 newlines and print the substring between the two. That can be done in C code by checking for the loader, checking for get_source(), calling it, getting the char buffer, and then just looping through looking for the needed newlines to find the needed indexes. Otherwise we can use the Python API on strings to do what would have been done in pure Python, but that is a lot of PyObject_Call() usage and seems overly inefficient if one bothers with coding it in C. =) |
|
|
| msg58110 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2007-12-03 04:19 |
| The attached patch implements warn_explicit(), all in C. Tests are still needed, though (including for showwarning() and formatwarning() to deal with their new 'line' argument). And -W still needs to be dealt with. |
|
|
| msg62708 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2008-02-22 23:36 |
| Patch that has been brought up-to-date with r60968. No new work, though. |
|
|
| msg63510 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2008-03-13 20:07 |
| Add tests for the 'line' argument to formatwarning() and showwarning(). |
|
|
| msg64040 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2008-03-19 05:14 |
| Attached is what I think is a completely working version of warnings implemented in a mixture of C and Python. I am not worrying about documenting the new C APIs I had to add since they are pretty specific to internal stuff. Probably should add a leading '_', but I'm tired. =) I think the fleshed out tests do a pretty good job of testing new code. Even if this patch gets held up the tests should definitely get backported as is reasonable. Assigning to Neal to review (hopefully soon). |
|
|
| msg64780 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2008-03-31 10:11 |
| On another note, the warnings module should be made to work with or without _warnings. that way IronPython, Jython, and PyPy won't have to re- implement stuff. This also means that test cases need to be changed to test this. |
|
|
| msg64801 - (view) |
Author: Neal Norwitz (nnorwitz) * |
Date: 2008-04-01 04:48 |
| I didn't realize this was waiting for me. You should have just checked it in, that would have gotten me to review faster. :-) pythonrun.c: * Should PyModule_GetWarningsModule() return a valid pointer? * The code below crashes. Need to XDECREF, not DECREF (or similar). + PyObject *warnings_module = PyImport_ImportModule("warnings"); + if (!warnings_module) + PyErr_Clear(); + Py_DECREF(warnings_module); Python/_warnings.c: * Remove // XXX(nnorwitz): need to parse -W cmd line flags Include/pythonrun.h * init_warnings has the wrong name (not prefixed with _Py). I'm not sure it should be exported at all. test_support/frozen: did you want the captured_std{out,err} change in this patch? Changes to Makefile.pre.in other than adding _warnings.o? I think this is good enough if it's working. How about checking it in after 1) the alpha is released Wed and 2) fixing up the nits? |
|
|
| msg64803 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2008-04-01 07:56 |
| After all the threats about checking in code that break stuff, I am not about to check this in. =) I will get to the changes when I can and then commit after the alpha. |
|
|
| msg64808 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2008-04-01 13:14 |
| Neal's issues are addressed in this patch. I also finally filled out warnings.h. The only thing that I didn't deal with is Neal's worry of exposing _PyWarnings_Init(). It is not explicitly exported anywhere as part of the API so I am not sure what he is worrying about. The attached patch is finished for CPython. I do want to go back and put back in the pure Python code that _warnings.c replaces so that alternative VMs don't have to implement any part of warnings if they don't want to. |
|
|
| msg64820 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2008-04-01 20:07 |
| Attached should have everything, including a pure Python fallback. As soon as the next alpha is out I will apply. |
|
|
| msg64831 - (view) |
Author: Neal Norwitz (nnorwitz) * |
Date: 2008-04-02 02:31 |
| On Tue, Apr 1, 2008 at 6:14 AM, Brett Cannon <report@bugs.python.org> wrote: > > Brett Cannon <brett@python.org> added the comment: > > Neal's issues are addressed in this patch. I also finally filled out > warnings.h. The only thing that I didn't deal with is Neal's worry of > exposing _PyWarnings_Init(). It is not explicitly exported anywhere as > part of the API so I am not sure what he is worrying about. I wasn't so worried about exposing it, I didn't like the name pollution (we're talking about init_warnings, right?). I know that we have other modules that use init*, but those are broken too. I'm not sure we should fix those in 2.6, but 3.0 we should. |
|
|
| msg65427 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2008-04-12 23:44 |
| Committed in revision 62303. |
|
|