Skip to content

Ensure [[:blank:]] only matches [ \t] #534

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 1 commit into from
Oct 29, 2018
Merged

Conversation

kennytm
Copy link
Member

@kennytm kennytm commented Oct 27, 2018

Fix #533.

@kennytm kennytm changed the title Ensure [[:blank:]] only matches [ \t]. Fix #533. Ensure [[:blank:]] only matches [ \t] Oct 27, 2018
Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

Nice catch! I think [[:space:]] is actually supposed to match more than [ \t] though.

@@ -1040,7 +1040,7 @@ fn ascii_class(kind: &ast::ClassAsciiKind) -> &'static [(char, char)] {
X
}
Blank => {
const X: T = &[(' ', '\t')];
const X: T = &[('\t', '\t'), (' ', ' ')];
Copy link
Member

Choose a reason for hiding this comment

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

Oof. Thanks for catching this! It looks like a bad transcription error. However, I don't think this is right either. The docs include more characters:

[[:space:]]    whitespace ([\t\n\v\f\r ])

And this is consistent with how regex-syntax 0.4 and below behaved:

const SPACE: Class = &[('\t', '\t'), ('\n', '\n'), ('\x0B', '\x0B'),
('\x0C', '\x0C'), ('\r', '\r'), (' ', ' ')];

Copy link
Contributor

Choose a reason for hiding this comment

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

@BurntSushi You're referring to docs for space, but this bug and fix are for blank which has fewer characters.

Copy link
Member

Choose a reason for hiding this comment

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

Ug. Thanks for that. I don't think I had quite woken up yet!

Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

Thanks to @RReverser for fixing my errant observation! With that, this looks good!

@BurntSushi BurntSushi merged commit 5241919 into rust-lang:master Oct 29, 2018
@kennytm kennytm deleted the fix-533 branch October 29, 2018 12:56
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