classification
Title: redundant checks in tok_get in Parser\tokenizer.c
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Oren Milman, SilentGhost, benjamin.peterson, martin.panter, matrixise, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2016-01-10 18:59 by Oren Milman, last changed 2016-03-25 05:43 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
tokenizer.c.withoutRedundantChecks.c Oren Milman, 2016-01-10 18:59 the patched tokenizer.c
issue26076.diff Oren Milman, 2016-01-10 22:16 hg diff file review
Messages (9)
msg257927 - (view) Author: Oren Milman (Oren Milman) * Date: 2016-01-10 18:59
In Parser\tokenizer.c, in tok_get, in the identification of a potential NUMBER token, in case the token starts with a '0', the next char of the token is retrieved, followed by two redundant checks:
if (c == '.')
    goto fraction;
if (c == 'j' || c == 'J')
    goto imaginary;

These two are redundant, because they check for the case of a token starting with '0.' or '0j', but even without them, the flow in either of these would reach the code after the /* maybe old-style octal; c is first char of it */ comment.
This code (after consuming all zeros and all decimal digits) would again perform those exact two checks, and handle them exactly the same.

My proposal is simply to remove the first two checks.

I have attached the patched tokenizer.c (the redundant checks are just commented out).
msg257932 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2016-01-10 20:27
Hi SilentGhost and Oren,

There is a problem with this patch. This is not a patch, but the
complete file. 

1. Could you provide a real patch, with the real diff. Use hg diff >
/tmp/issue26076.diff

2. Please, could you provide a small test to be sure that everything
goes well.

Thank you,

Stephane
msg257933 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2016-01-10 20:29
The patch indeed would be more useful, though I'd suggest to actually delete the lines you think should be deleted, rather than just leave the commented out.
msg257934 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2016-01-10 20:32
Totally agree with you, 

1. Create a patch
2. Remove the commented lines in the patch
msg257943 - (view) Author: Oren Milman (Oren Milman) * Date: 2016-01-10 22:16
Sorry for being so clueless.
The diff is attached.

I manually did some checks to verify that relevant stuff work correctly (the imaginary number 0j, and fractions starting with '0.').
I run 'python -m test', and got the following output:
352 tests OK.
1 test altered the execution environment:
    test_subprocess
45 tests skipped:
    test_bz2 test_crypt test_curses test_dbm_gnu test_dbm_ndbm
    test_devpoll test_epoll test_fcntl test_fork1 test_gdb test_grp
    test_idle test_ioctl test_kqueue test_lzma test_nis test_openpty
    test_ossaudiodev test_pipes test_poll test_posix test_pty test_pwd
    test_readline test_resource test_smtpnet test_socketserver
    test_spwd test_sqlite test_ssl test_syslog test_tcl
    test_threadsignals test_timeout test_tix test_tk test_ttk_guionly
    test_ttk_textonly test_urllib2net test_urllibnet test_wait3
    test_wait4 test_winsound test_xmlrpc_net test_zipfile64

Then, I removed my patch (and made sure hg diff output nothing), rebuilt CPython, run 'python -m test', and got exactly the same output.
msg257948 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2016-01-11 03:29
Thank you for your patch, it's easier for the review.
msg262331 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2016-03-24 10:38
Could any one of the core developers have a look? Seems like a rather straightforward change.
msg262392 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-03-25 00:37
The change looks pretty good to me.

I guess some test cases could be added to Lib/test/test_grammar.py, unless there is somewhere else that already tests this sort of stuff.
msg262402 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-03-25 05:43
New changeset f0acce8022d1 by Benjamin Peterson in branch 'default':
remove duplicated check for fractions and complex numbers (closes #26076)
https://hg.python.org/cpython/rev/f0acce8022d1
History
Date User Action Args
2016-03-25 05:43:58python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg262402

resolution: fixed
stage: patch review -> resolved
2016-03-25 00:37:12martin.pantersetnosy: + martin.panter
messages: + msg262392
2016-03-24 10:38:27SilentGhostsetmessages: + msg262331
2016-01-11 03:29:07matrixisesetmessages: + msg257948
2016-01-11 03:27:19matrixisesetnosy: + serhiy.storchaka
2016-01-10 22:25:23SilentGhostsetversions: - Python 2.7, Python 3.2, Python 3.3, Python 3.4, Python 3.5
2016-01-10 22:16:44Oren Milmansetfiles: + issue26076.diff

messages: + msg257943
versions: + Python 2.7, Python 3.2, Python 3.3, Python 3.4, Python 3.5
2016-01-10 20:32:10matrixisesetmessages: + msg257934
2016-01-10 20:29:44SilentGhostsetnosy: + SilentGhost
messages: + msg257933
2016-01-10 20:27:43matrixisesetnosy: + matrixise
messages: + msg257932
2016-01-10 19:14:14SilentGhostsetkeywords: + patch
nosy: + benjamin.peterson
stage: patch review

versions: - Python 2.7, Python 3.2, Python 3.3, Python 3.4, Python 3.5
2016-01-10 18:59:59Oren Milmancreate