Skip to content

_match: correct max_slice_length logic #37603

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

Merged
merged 2 commits into from
Nov 10, 2016
Merged

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Nov 5, 2016

The logic used to be wildly wrong, but before the HAIR patch its wrongness was in most cases hidden by another bug.

Fixes #37598.

r? @nikomatsakis

The logic used to be wildly wrong, but before the HAIR patch its
wrongness was hidden by another bug.

Fixes rust-lang#37598.
//
// This means that all patterns with length at least
// `max(max_fixed+1,max_prefix+max_suffix)` are equivalent, so we
// only need to check patterns from that length and below.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good comment!

A minor note: I think it would be marginally improved by a code example, showing what (e.g.) max_fixed etc mean in Rust code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I am a bit confused -- why is it max_fixed_len + 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It must be strictly larger than max_fixed, and >= max_fixed + max_suffix.

// Each slice `s` with a "sufficiently-large" length `l ≥ L` that applies
// to exactly the subset `Pₜ` of `P` can be transformed to a slice
// `sₘ` for each sufficiently-large length `m` that applies to exactly
// the same subset of `P`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is now even more awesome =) But I admit I am kind of getting lost a bit. It would have been helpful (for me) to note that these are byte slices, and that -- effectively -- b"123" is equivalent to &[b'1', b'2', b'3'].

And then maybe some examples like:

match foo { // foo : &[u8]
    b"123" => ..., // matches byte strings of length 3
    b"1234" => ..., // matches byte strings of length 4
    &[b'1', ..c, b'9'] => , // matches byte strings of length 2..infinity
} // here `max_slice_check` yields 5, as `max_fixed_len` is 4, `max_prefix_len` is 1, `max_suffix_len` is 1
match foo { // foo : &[u8]
    b"123" => ..., // matches byte strings of length 3
    b"1234" => ..., // matches byte strings of length 4
    &[b'1', b'2',  ..c, b'7', b'8, b'9'] => , // matches byte strings of length 5..infinity
} // here `max_slice_check` again yields 5, as `max_fixed_len` is 4, `max_prefix_len` is 2, `max_suffix_len` is 3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are not byte slices, in general (see examples with boolean slices and Option<()> slices).

If there is a byte literal, then as usual it is treated like the expanded byte slice.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Nov 9, 2016

📌 Commit 1dad4b6 has been approved by nikomatsakis

@bors
Copy link
Collaborator

bors commented Nov 9, 2016

⌛ Testing commit 1dad4b6 with merge 0b46947...

bors added a commit that referenced this pull request Nov 9, 2016
_match: correct max_slice_length logic

The logic used to be wildly wrong, but before the HAIR patch its wrongness was in most cases hidden by another bug.

Fixes #37598.

r? @nikomatsakis
@bors bors merged commit 1dad4b6 into rust-lang:master Nov 10, 2016
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.

3 participants