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.

classification
Title: Cosmetic issues that may warrant a fix in symtable.h/c
Type: behavior Stage: commit review
Components: Interpreter Core Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: eli.bendersky, eric.araujo, ncoghlan, python-dev, terry.reedy
Priority: normal Keywords:

Created on 2010-09-20 06:29 by eli.bendersky, last changed 2022-04-11 14:57 by admin. This issue is now closed.

Messages (10)
msg116911 - (view) Author: Eli Bendersky (eli.bendersky) * (Python committer) 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) * (Python committer) 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: Nick Coghlan (ncoghlan) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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: Nick Coghlan (ncoghlan) * (Python committer) 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) (Python triager) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2011-04-16 03:50
Thanks for the clarification, Éric
History
Date User Action Args
2022-04-11 14:57:06adminsetgithub: 54113
2011-04-16 03:50:59eli.benderskysetmessages: + msg133879
2011-04-15 17:11:51eric.araujosetnosy: + eric.araujo
messages: + msg133850
2011-04-15 04:17:07eli.benderskysetstatus: pending -> closed
resolution: fixed
2011-04-10 04:34:41eli.benderskysetstatus: open -> pending

messages: + msg133446
stage: needs patch -> commit review
2011-04-10 04:33:07python-devsetnosy: + python-dev
messages: + msg133445
2011-04-10 04:04:26eli.benderskysetstage: needs patch
versions: + Python 3.3, - Python 3.2
2011-04-09 10:07:09ncoghlansetmessages: + msg133380
2011-04-08 20:55:36terry.reedysetmessages: + msg133342
2011-04-08 18:42:57eli.benderskysetmessages: + msg133334
2011-01-09 07:06:47ncoghlansetnosy: terry.reedy, ncoghlan, eli.bendersky
messages: + msg125825
2011-01-08 21:45:40terry.reedysetnosy: + terry.reedy
messages: + msg125802
2010-09-20 06:29:33eli.benderskysettitle: Cosmetic issues that might be worthy a fix in symtable.h/c -> Cosmetic issues that may warrant a fix in symtable.h/c
2010-09-20 06:29:04eli.benderskycreate