| msg116911 - (view) |
Author: Eli Bendersky (eli.bendersky) *  |
Date: 2010-09-20 06:29 |
| The following minor issues may affect the readability of the code implementing symbol tables in Include/symtable.h and Python/symtable.c * The comment for st_global in symtable.h says: "borrowed ref to st_top->st_symbols. typo? (st_top->ste_symbols) * ste_varnames: the name and the comment after it are misleading, since it actually collects only the function's parameters and not all variables. * the st_nblocks and st_future fields of symtable aren't used anywhere - py3k compiles fine when they're removed. * in analyze_block a comment says "Recursively call analyze_block()" - untrue, probably meant analyze_child_block. While technically analyze_child_block calls analyze_block, the comment as-is appears misleading. * symtable_add_def is also called with the USE flag, not only definitions, hence its name doesn't reflect its purpose accurately * There are some indentation artifacts that obscure readability. For example the case Raise_kind of symtable_visit_stmt, where two nested blocks start and end in the same column obscuring the fact they're nested. This could be a result of an automatic tab to space conversion in the past. |
|
|
| msg125802 - (view) |
Author: Terry J. Reedy (terry.reedy) * |
Date: 2011-01-08 21:45 |
| I think this needs at least two patches: one to change comments, another to remove two fields. Can you prepare something? |
|
|
| msg125825 - (view) |
Author: Alyssa Coghlan (ncoghlan) * |
Date: 2011-01-09 07:06 |
| If I recall correctly, st_nblocks and st_future are there for consistency with the corresponding compiler structures. I believe st_future is also needed whenever a future flag affects the symtable construction (there aren't any at the moment, but a trawl through the version history in the Python 2.x symtable.c may reveal some) |
|
|
| msg133334 - (view) |
Author: Eli Bendersky (eli.bendersky) * |
Date: 2011-04-08 18:42 |
| Terry, Nick - is it OK to commit a fix for the comments? |
|
|
| msg133342 - (view) |
Author: Terry J. Reedy (terry.reedy) * |
Date: 2011-04-08 20:55 |
| I would say yes (if you are sure changes are correct) unless Nick speaks up to say he wants to review first. |
|
|
| msg133380 - (view) |
Author: Alyssa Coghlan (ncoghlan) * |
Date: 2011-04-09 10:07 |
| +1 for clarifying the comments. Adding comments for the apparently unused fields (as per my last tracker comment) would be good, too. |
|
|
| msg133445 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2011-04-10 04:33 |
| New changeset b6fe63c914e4 by Eli Bendersky in branch 'default': Issue #9904: fix and clarify some comments + fix indentation in symtable code http://hg.python.org/cpython/rev/b6fe63c914e4 |
|
|
| msg133446 - (view) |
Author: Eli Bendersky (eli.bendersky) * |
Date: 2011-04-10 04:34 |
| I committed the fixes to 3.3 (see no reason to backport these). So unless there are objections I will close the issue in a few days. |
|
|
| msg133850 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2011-04-15 17:11 |
| Yep, code cleanup is not done in the stable branches (except as a by-product of a bugfix). |
|
|
| msg133879 - (view) |
Author: Eli Bendersky (eli.bendersky) * |
Date: 2011-04-16 03:50 |
| Thanks for the clarification, Éric |
|
|