Skip to content

bpo-31671: re.compile() cache checks flags type#3867

Closed
vstinner wants to merge 3 commits intopython:masterfrom
vstinner:re_compile_flags_type
Closed

bpo-31671: re.compile() cache checks flags type#3867
vstinner wants to merge 3 commits intopython:masterfrom
vstinner:re_compile_flags_type

Conversation

@vstinner
Copy link
Copy Markdown
Member

@vstinner vstinner commented Oct 3, 2017

Make sure that float flags are rejected even if the cache has an
entry with equal flags but flags of a different type.

https://bugs.python.org/issue31671

Make sure that float flags are rejected even if the cache has an
entry with equal flags but flags of a different type.
Comment thread Lib/re.py Outdated
_MAXCACHE = 512
def _compile(pattern, flags):
# internal: compile pattern
cache_key = (type(pattern), pattern, type(flags), flags)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Putting type(flags) in the cache key prevents to merge flags=2 with flags=re.IGNORECASE.

I suggest to first focus on correctness. I let @methane handles the performance part with his PR #3862 :-)

But I propose to backport my fix to 2.7 and 3.6 without the optimization, so a minor performance regression for Python 3.6.

@python python deleted a comment from kumbiepython Oct 3, 2017
Copy link
Copy Markdown
Member

@methane methane left a comment

Choose a reason for hiding this comment

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

LGTM

@vstinner
Copy link
Copy Markdown
Member Author

vstinner commented Oct 5, 2017

I abandon my change for these reasons: https://bugs.python.org/issue31671#msg303753

@vstinner vstinner closed this Oct 5, 2017
@vstinner vstinner deleted the re_compile_flags_type branch October 5, 2017 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants