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 Eq for Regex #81

Merged
merged 3 commits into from Apr 23, 2015
Merged

Implement Eq for Regex #81

merged 3 commits into from Apr 23, 2015

Conversation

ghost
Copy link

@ghost ghost commented Apr 23, 2015

Note that this is just a comparison of the original strings, it's still possible that two different strings result in the same matching behaviour.

Note that this is just a comparison of the original strings, it's
still possible that two different strings result in the same matching
behaviour.
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @huonw (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTING.md for more information.

@BurntSushi
Copy link
Member

I don't think there's any feasible way to detect whether two arbitrary expressions match the same set of strings, so I'm OK with this definition.

@@ -159,6 +159,14 @@ impl fmt::Debug for Regex {
}
}

impl PartialEq for Regex {
Copy link
Member

Choose a reason for hiding this comment

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

@yorhel Could you please put a doc comment on this impl explaining the definition of equality used? (Something like what you said in your initial comment is just fine.)

@BurntSushi
Copy link
Member

I have one nit, but other than that, looks good!

@BurntSushi BurntSushi assigned BurntSushi and unassigned huonw Apr 23, 2015
Yorhel added 2 commits April 23, 2015 19:04
(Sorry for the noise, didn't even realize "behaviour" was British
spelling)
@ghost
Copy link
Author

ghost commented Apr 23, 2015

Doc added. Thanks for the feedback!

BurntSushi added a commit that referenced this pull request Apr 23, 2015
@BurntSushi BurntSushi merged commit 0709ef1 into rust-lang:master Apr 23, 2015
@BurntSushi
Copy link
Member

Awesome. Good docs! Thanks!

@huonw
Copy link
Member

huonw commented Apr 27, 2015

I don't think there's any feasible way to detect whether two arbitrary expressions match the same set of strings, so I'm OK with this definition.

I looked into it a bit, and yes, it seems to be non-trivial: http://cs.stackexchange.com/q/12267 (that said, it is stated that one can test equality by computing minimal DFAs and comparing).

@BurntSushi
Copy link
Member

@huonw Yeah, when we get DFAs (#66) then maybe we can expose that sort of operation, but it probably shouldn't be in an impl of PartialEq, which most would expect to be fast. (Computing the DFA can take quite a lot of time!)

Even better, maybe @carllerche's automaton package can do it for us.

@carllerche
Copy link
Member

Automaton currently computes the minimal DFA using hopcroft's algorithm. Of course, there could always be bugs and the lib is still under heavy development 😄

@ghost ghost unassigned BurntSushi Sep 21, 2015
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.

4 participants