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

Implement named backreferences #433

Merged
merged 2 commits into from
May 25, 2022
Merged

Conversation

hamishknight
Copy link
Contributor

Use the CaptureList as the source of truth on which index a name corresponds to, and query it when emitting a named backreference.

Resolves #388

@hamishknight
Copy link
Contributor Author

@swift-ci please test

@hamishknight
Copy link
Contributor Author

Hmm, removing namedCaptureOffsets from MECaptureList appears to be causing seemingly unrelated tests to crash on macOS CI (but not locally for me), any ideas what might be going on there?

@rxwei
Copy link
Contributor

rxwei commented May 24, 2022

@swift-ci please test macOS

@rxwei
Copy link
Contributor

rxwei commented May 25, 2022

looks like a CI glitch!

Copy link
Contributor

@rxwei rxwei left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@milseman milseman left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -38,13 +38,24 @@ extension MEProgram where Input.Element: Hashable {
// Special addresses or instructions
var failAddressToken: AddressToken? = nil

var captureList = CaptureList()
private(set) var namedCaptureOffsets: [String: Int] = [:]
Copy link
Member

Choose a reason for hiding this comment

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

Could we just fully drop the dictionary? If there are fewer than 2^4-ish named captures I'd expect linear search to be better anyways. It seems like CaptureList could just have an API for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, how does this look?

Use the CaptureList as the source of truth on
which index a name corresponds to, and query it
when emitting a named backreference.
This doesn't appear to be used, and should be
available from the CaptureList.
@hamishknight hamishknight force-pushed the named-refs branch 2 times, most recently from a9fcc86 to 4b7d534 Compare May 25, 2022 13:09
@hamishknight
Copy link
Contributor Author

@swift-ci please test

@hamishknight hamishknight merged commit 471e073 into swiftlang:main May 25, 2022
@hamishknight hamishknight deleted the named-refs branch May 25, 2022 16:07
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.

Implement named backreference support
3 participants