Issue1549670
This issue tracker has been migrated to GitHub,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2006-08-31 01:06 by jiwon, last changed 2022-04-11 14:56 by admin. This issue is now closed.
Files | ||||
---|---|---|---|---|
File name | Uploaded | Description | Edit | |
pep3102-final.patch | jiwon, 2006-11-22 02:44 | Now the compiler package correctly parses the grammar for pep3102. |
Messages (14) | |||
---|---|---|---|
msg51023 - (view) | Author: Jiwon Seo (jiwon) * | Date: 2006-08-31 01:06 | |
This patch is implementation of pep 3102, keyword-only parameter. Important changes include * code object now has co_kwonlyargcount to keep the number of keyword only argument - this is analogous to co_argcount. * function object now has func_kwdefaults (dictionary) to keep the mapping between keyword only arguments and defaults for them. Only kwonly argument with default values are in the dictionary - this is analogous to func_defaults. * APIs for code object changed - both C API and Python Api. PyCode_New now takes number of keyword only arguments, and new.code also takes number of keyword only arguments. * MAKE_FUNCTION now takes another oparg, which is number of default keyword only arguments - and the name of keyword only argument and its default value are in the value stack - it is similar to oparg of CALL_FUNCTION. * MAGIC in import.c changed, since bytecode is changed. That's pretty much everything that's important, and the rest is in the code itself. And my patch passes all regression tests excepts test_frozen.py, which depends on the hard-coded mashal value, which needs to be regenerated when bytecode changes. However, freeze.py is broken - specifically, modulefinder.py is broken as Guido said. So, currently, I commented it out. |
|||
msg51024 - (view) | Author: Neal Norwitz (nnorwitz) * | Date: 2006-08-31 05:42 | |
Logged In: YES user_id=33168 import.c: comment has double word 'added' ast.c: * why is static commented out for ast_for_arguments? * in handle_keywordonly_args, doesn't the default case need to return -1 after setting the error? * need to change the // comments to /* */ * it looks like a bunch of lines had whitespace, but no other changes. this makes it hard to see the real changes. compile.c: * i think there is a bug in the arglength calc if there are more than 255 positional args and kw_default_count since there is |= kw_default_count << 8 (2 places) * return should go on its own line like the rest of the surrounding code. * why kwonlyargs and kw_args? the underscore seems inconsistent in the compiler package, I didn't see a change to MAKE_FUNCTION similar to the one in compile.c for the opcode stack effect. the change to regrtest.py doesn't seem necessary (just an added blank line) there should be a lot more tests added for both positive and negative conditions (syntax errors). Include variants with func(**kwargs) func(*args), etc in calls. it's not good to comment out the tests in test_frozen. otherwise, we will probably forget about the problems. when you say it passes all tests, did you run with -u all? the compiler doesn't do all the tests without it (and one or two other tests too). in the new test, i think you need to pass a name for the filename param to compile. I think there was some platform (windows?) that crapped out with an empty filename. (if you copied from an existing test, then i must be remembering something different.) In testRaiseErrorWhenFuncall(), you can use self.assertRaises, at least for most of the cases. you need a test_main for this test to be run from regrtest. |
|||
msg51025 - (view) | Author: Jiwon Seo (jiwon) * | Date: 2006-08-31 21:03 | |
Logged In: YES user_id=595483 >> * it looks like a bunch of lines had whitespace, but no >> other changes. this makes it hard to see the real changes. I changed some tabs in ast_for_argument function to spaces - tabs and spaces are mixed, and it's hard to edit. It would have been nice if I separate that white space conversion into another patch, but since it's p3yk branch I thought it can be allowed. >> when you say it passes all tests, did you run with -u all? >> the compiler doesn't do all the tests without it (and one >> or >> two other tests too). Yup, it passes all the tests except test_frozen and test_linuxaudiodev.py(maybe because my box doesn't have audio device..?) and some skipped tests including test_bsddb3.py, etc >> in the new test, i think you need to pass a name for the >> filename param to compile. I think there was some platform >> (windows?) that crapped out with an empty filename. (if you >> copied from an existing test, then i must be remembering >> something different.) Yeah, I copied it from test_with.py, but now I'm passing a filename. |
|||
msg51026 - (view) | Author: Jiwon Seo (jiwon) * | Date: 2006-08-31 21:07 | |
Logged In: YES user_id=595483 Maybe I should mention that I've uploaded new patch that fixed what Neal mentioned. |
|||
msg51027 - (view) | Author: Jiwon Seo (jiwon) * | Date: 2006-09-07 17:58 | |
Logged In: YES user_id=595483 The original patch crashes when a function has keyword only arguments but no default values for those arguments, and calls the function without enough arguments. Fixed the bug, and added testcases for that. |
|||
msg51028 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2006-09-11 19:51 | |
Logged In: YES user_id=6380 General comment: I think you should update or add a comment explaining the contents of the code object as it has changed (unless that's all described in the PEP). There are a number of places where the code is wider than 79 characters or where the indentatiln style seems to be off. You may have to set your tabstops to 8 spaces to see these. E.g.: ceval.c: - line 2624 should be indented with next line - line 2680 is too long - @@ -2694,13 +2715,35 @@ several lines too long - line 3618 should be indented with the next line ast.c: @@ -591,6 +591,63 @@ please follow the surrounding 4-space indents; don't use tabs here marshal.c: line 871 too long codeobject.c: several lines too long funcobject.c: too long at "non-dict keyword only default args"); Lib/compiler/*.py: several lines too long Lib/test/test_new.py Modules/pyexpat.c: @@ -279,6 +279,7 @@ indentation error Modules/parsermodule.c: several lines too long; the loop at "while (res && i+1 < nch)" is indented one tabstop too many (you seem to have edited this file with ts=4?) |
|||
msg51029 - (view) | Author: Jiwon Seo (jiwon) * | Date: 2006-09-13 21:16 | |
Logged In: YES user_id=595483 The codeobject has a kwonlyargcount meaning the number of keyword only arguments. Thus, the api for creating new code object is changed: PyCode_New now takes kwonlyargcount. So, the signature changes from this PyCode_New(argcount, nlocals, stacksize, ...) to following. PyCode_New(argcount, kwonlyargcount, nlocals, stacksize, ...) From python, you can access this with name "co_kwonlyargcount" such as, co.co_kwonlyargcount just like you access argcount with co.co_argcount. |
|||
msg51030 - (view) | Author: Jiwon Seo (jiwon) * | Date: 2006-09-14 07:04 | |
Logged In: YES user_id=595483 I made a big mistake with the grammar - currently, the grammar doesn't allow keyword only argument after *vararg. At some point of the development process, I just assumed as such. Underlying logic does not assume anything about that, so I'll try to fix it as soon as possible |
|||
msg51031 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2006-09-15 17:13 | |
Logged In: YES user_id=6380 I see. The Grammar should simply make the NAME after the '*' optional. Shouldn't be too hard to fix should it? |
|||
msg51032 - (view) | Author: Jiwon Seo (jiwon) * | Date: 2006-10-09 22:22 | |
Logged In: YES user_id=595483 The new patch that I am uploading fixes the Grammar problem - originally I made mistake not allowing *vararg after keyword only argument. The python compiler package doesn't work now, but everything else should work now. |
|||
msg51033 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2006-10-27 23:32 | |
Logged In: YES user_id=6380 Checked in as r52491. I'm keeping this open awaiting a patch for the compiler package. |
|||
msg51034 - (view) | Author: Jiwon Seo (jiwon) * | Date: 2006-11-22 02:44 | |
Now the compiler package works. The patch passes regrtest with "-u all" option and "-u compiler" option (except test_frozen.py). The patch is against latest p3yk branch HEAD. The last patch accidentally included some test code in test_compiler.py (and which is committed to the svn) so this patch includes fix for that. Ah, Guido, when you committed the patch (previous version) last time, you did not add Lib/test/test_keywordonlyarg.py so, this time, don't forget to add it :) Also, last time when we talked, I think you said that the patch makes some test case fail on your laptop machine. You might want to check that. |
|||
msg51035 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2006-11-22 04:56 | |
Submitted. Jiwon, can you mail me another copy of test_keywordinoyarg.py? I believe I lost it. |
|||
msg51036 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2006-11-23 01:18 | |
All submitted. (test_keywordonlyarg.py too) |
History | |||
---|---|---|---|
Date | User | Action | Args |
2022-04-11 14:56:19 | admin | set | github: 43917 |
2008-01-06 22:29:46 | admin | set | keywords:
- py3k versions: + Python 3.0 |
2006-08-31 01:06:34 | jiwon | create |