Skip to content
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

Extract internal Regex API for PCRE backend #12802

Merged

Conversation

straight-shoota
Copy link
Member

@straight-shoota straight-shoota commented Nov 29, 2022

This patch extracts code that relates directly to implementation details of the libpcre backend from Regex and Regex::MatchData and puts it into a module Regex::PCRE.

This abstraction is then to be used for an alternative engine using PCRE2 (#12790).

The first two commits are an separate change submitted in #12810.

Commit three and fourone and two are general refactors which are useful on their own and make the extraction easier.

@straight-shoota
Copy link
Member Author

straight-shoota commented Nov 29, 2022

So there was actually a bug in the original refactor that didn't get caught by the spec suite, despite its huge expansion. I added some missing test cases in #12804 (also first commit in this branch).

@straight-shoota straight-shoota force-pushed the refactor/regex-interface branch 2 times, most recently from c087ce6 to ca016bd Compare November 29, 2022 22:08
@straight-shoota straight-shoota marked this pull request as draft November 30, 2022 12:15
@straight-shoota straight-shoota force-pushed the refactor/regex-interface branch 4 times, most recently from 4d50cf9 to a3383de Compare December 1, 2022 00:12
@straight-shoota straight-shoota marked this pull request as ready for review December 2, 2022 10:19
src/regex/pcre.cr Show resolved Hide resolved
src/regex/engine.cr Show resolved Hide resolved
@straight-shoota straight-shoota self-assigned this Dec 8, 2022
Copy link
Member

@beta-ziliani beta-ziliani left a comment

Choose a reason for hiding this comment

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

Nothing worth blocking 👍

# :nodoc:
module Regex::PCRE
# :nodoc:
def initialize(*, _source source, _options @options)
Copy link
Member

Choose a reason for hiding this comment

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

Why not make this one private too? Also, wouldn't it make sense to change its name, and avoid the renaming of arguments?

Copy link
Member Author

@straight-shoota straight-shoota Dec 12, 2022

Choose a reason for hiding this comment

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

Yeah, I think private should work. I think the nodoc is an artifact from an earlier setup where private wouldn't have worked.

Changing the name would make this not be a constructor which causes problems with ivar initialization.

@straight-shoota straight-shoota added this to the 1.7.0 milestone Dec 12, 2022
@straight-shoota straight-shoota merged commit c8f1af5 into crystal-lang:master Dec 14, 2022
@straight-shoota straight-shoota deleted the refactor/regex-interface branch December 14, 2022 11:07
carlhoerberg pushed a commit to carlhoerberg/crystal that referenced this pull request Dec 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants