-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bpo-46920: Remove code made dead long ago with #if 0 #31681
Conversation
There is more potential dead code, though I don't know if it can be safely removed.
|
I don't think all of these should be removed. Some of them look genuinely useful for debugging, others are essentially multi-line comments. In any event, they should be individually considered. |
@ericvsmith I agree that the debugging share of code appeared for a reason so I'm open to evaluation and ready to exclude parts from the PR. BTW I decided to remove that potentially-not-dead share because it either requires some missing harness or looks finished to serve its time after a tested feature was stabilized long ago. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
*/ | ||
#if 0 | ||
_PyGC_CollectIfEnabled(); | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spent significant time to rework how the GC is called. The last call is now in interpreter_clear() of pystate.c.
See: https://pythondev.readthedocs.io/finalization.html
I often work on the Python initialization and finalization code and I never needed to disble the |
* XXX the exception and consequent unexpected failures). I've also | ||
* XXX seen segfaults then, after adding print statements to the | ||
* XXX Python code getting called. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is oudated and must be removed.
@mdickinson @ericvsmith: Would you mind to review again the PR? If would prefer a second approval since there was disagreement on the first reviews. |
Python/compile.c
Outdated
/* For debugging purposes only */ | ||
#if 0 | ||
static void | ||
dump_instr(struct instr *i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These have been useful occasionally, so I would rather not remove them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restored.
Re: fastsearch.h, I'd like to keep those debugging prints. If a bug or performance curiosity ever shows up, it would be nice to flip the 0 to a 1, recompile, and then be able to tell exactly what's going on, step-by-step. |
I don't have any strong opinion on any of these changes besides the vendorised code changes that were already addressed (for which thank you, @arhadthedev!). I'm personally happy for the rest of the changes to go through, but other people's concerns should be taken into account. |
No. The comment is simply wrong: it's testing the behavior of "if 0:" in Python, nothing to do with "#if 0" in C. |
Except for ones that are out of date, wrong, or misleading, I don't see much need in deleting any of these. |
I found three more functions disabled by commenting them out. Since they were put into a comment from the moment of being introduced, I suspect they are obsolete so removed them too:
|
It would be easier to review this if you opened new issues when you found other code you want removed. Ideally (for me, at least), each piece of code you want to remove would be in a separate PR, so they could be easily reviewed and merged. I'm not sure if it's worth doing that at this point, but because there are so many files that are touched in this PR, I don't think I can give it a proper review. |
Initially I didn't want to create a dosen of small PRs but now I see your point. I also got it that small PRs allow to get the most of pieces merged sooner without being blocked by more ambivalent ones. I'll make the split tomorrow. |
@ericvsmith I split this PR into smaller ones to not block merging of non-controversial pieces: GH-31811, GH-31812, GH-31813, and GH-31814. cc: @mdickinson |
@vstinner Thanks for merging all the commits. |
git grep "#if 0"
gives the following occurences of dead code (analyzed withgit blame
, removed by this commit):added on 27 Apr 2020 by 2b74c83:
cpython/Parser/pegen.h
Lines 9 to 11 in 8f31bf4
cpython/Parser/pegen.h
Lines 15 to 19 in 8f31bf4
Since these constants aren't mentioned anywhere else, I think it's some initial experiment now abandoned.
added on 23 Apr 2020 by c5fc156:
cpython/Parser/pegen.c
Lines 40 to 49 in 8f31bf4
token_name
is mentioned nowhere in the CPython codebase too.added on 20 Nov 2014 by d600951:
cpython/Python/pylifecycle.c
Lines 1843 to 1845 in 8f31bf4
with the following note:
So the call is faulty for seven years with no progress.
added on 26 Oct 2013 by 8444ebb:
cpython/Modules/sre_lib.h
Lines 336 to 362 in 8f31bf4
The Secret Labs' Regular Expression Engine was vendored nine years ago so
not used in this release
is permanent and can be removed.added on 28 Sep 2011 by d63a3b8:
cpython/Python/formatter_unicode.c
Lines 140 to 156 in 8f31bf4
DEBUG_PRINT_FORMAT_SPEC
is used nowhere, plus putting a debugger breakpoint onto an interestingInternalFormatSpec
is more convenient.added on (already commented out) 11 Dec 2007 by a3534a6:
cpython/Objects/classobject.c
Lines 519 to 533 in 8f31bf4
added on 27 Aug 2007 by e226b55:
cpython/Objects/unicodeobject.c
Lines 13660 to 13666 in 8f31bf4
cpython/Objects/unicodeobject.c
Lines 14253 to 14256 in 8f31bf4
_decimal2ascii
is mentioned nowhere else in the CPython codebase.added (already commented out) on 8 Mar 2006 by d4c9320:
cpython/Modules/_ctypes/_ctypes.c
Lines 1420 to 1463 in 8f31bf4
added on 7 Dec 2005 by 8c8836b:
cpython/Modules/_elementtree.c
Lines 39 to 48 in 8f31bf4
In addition,
ALLOC()
andRELEASE()
are removed as unconditional no-ops.added on 4 Aug 2002 by 00f1e3f:
cpython/Parser/tokenizer.c
Lines 285 to 308 in 8f31bf4
/* Disable support for UTF-16 BOMs until a decision is made whether this needs to be supported */
- I have every reason to believe that the decision was Never Ever.added on 23 Mar 2002 by c24ea08:
cpython/Python/ceval.c
Lines 7065 to 7070 in 8f31bf4
The
/* future keyword */
is quite overdue already.added on 23 Jun 2001 by 01dfdb3:
cpython/Modules/getaddrinfo.c
Lines 41 to 58 in 8f31bf4
cpython/Modules/getnameinfo.c
Lines 37 to 49 in 8f31bf4
added on 14 Jul 1998 by 43ff868:
cpython/Modules/_tkinter.c
Lines 3443 to 3448 in 8f31bf4
git grep "#if 1"
gives the following:added on 20 Nov 2014 by d600951:
cpython/Python/pylifecycle.c
Line 2427 in 8f31bf4
After seven years, no trouble was found so this always-true guard has no use.
https://bugs.python.org/issue46920