Skip to content

optimize all_name_chars - #3442

Merged
benjaminp merged 1 commit into
masterfrom
benjamin-all_name_chars
Sep 8, 2017
Merged

optimize all_name_chars#3442
benjaminp merged 1 commit into
masterfrom
benjamin-all_name_chars

Conversation

@benjaminp

@benjaminp benjaminp commented Sep 8, 2017

Copy link
Copy Markdown
Contributor

Remove redundant PyUnicode_Check call. Use a static table for checking chars.

@benjaminp
optimize all_name_chars
99cd309 
Remove redundant PyUnicode_Check call. Use a static table for checking chars.
tiran
tiran approved these changes Sep 8, 2017
View reviewed changes

@tiran tiran left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@benjaminp
benjaminp merged commit 9020ac7 into master Sep 8, 2017
@benjaminp
benjaminp deleted the benjamin-all_name_chars branch September 8, 2017 01:09
@benjaminp

benjaminp commented Sep 8, 2017

Copy link
Copy Markdown
Contributor Author

For posterity, here's how I generated the table:

for i in range(128):
    if chr(i) in "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz":
        print "1,",
    else:
        print "0,",
    if i % 16 == 15:
        print

@tiran

tiran commented Sep 8, 2017

Copy link
Copy Markdown
Member

I take back my LGTM ... you seriously used Python 2 to generate the table? Shame on you! :)

@tiran

tiran commented Sep 8, 2017

Copy link
Copy Markdown
Member 
for i in range(128):
    print('{:d}, '.format(chr(i) in "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz"), end='')
    if i % 16 == 15:
        print()

@benjaminp

benjaminp commented Sep 8, 2017

Copy link
Copy Markdown
Contributor Author

It's just nostalgia.

@serhiy-storchaka

serhiy-storchaka commented Sep 8, 2017

Copy link
Copy Markdown
Member

LGTM too. I have found a bug in all_name_chars(), but this is different issue.

@serhiy-storchaka

serhiy-storchaka commented Sep 8, 2017

Copy link
Copy Markdown
Member

Only one nit: please update the comment before all_name_chars().

@serhiy-storchaka

serhiy-storchaka commented Sep 8, 2017

Copy link
Copy Markdown
Member

You could use !Py_ISALNUM(*s) && *s != '_' and get rid of ok_name_char.

@benjaminp benjaminp mentioned this pull request Sep 8, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tiran tiran tiran approved these changes

Assignees

No one assigned

Labels

skip issue skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@benjaminp @tiran @serhiy-storchaka @Mariatta @the-knights-who-say-ni @bedevere-bot