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

Fix to parse regex in default argument #5481



Copy link

An error happens on this code:

def foo(x = / /); end
$ crystal run
Syntax error in unexpected token: /

def foo(x = / /); end

This PR fixes it.

Copy link

asterite commented Jan 1, 2018

Should travis ci failures be ignored? Or do we know why they happen?

@makenowjust makenowjust force-pushed the fix/crystal/parse-regex-in-default-arg branch from 3844c3f to a528ddf Compare January 1, 2018 14:02
Copy link

RX14 commented Jan 1, 2018

CI failures shouldn't be ignored, since master works fine.

Copy link

asterite commented Jan 2, 2018

@makenowjust Looks good, thanks!

I think the slash_is_regex! thing is good, but I got it wrong that it belongs to the parser. The lexer should already know when a next token can be a regex or a division. So after the = token (ignoring any non-newline whitespace that comes next) / will never mean division, always regex.

So those slash_is_regex! calls should be in Lexer, not in Parser.

I plan to do this refactor later. But maybe we can just create an issue about this and if someone wants to tackle this they can go ahead (it could be @makenowjust but it could be anyone, really). It should be relatively simple given that there are plenty of tests for crystal's parser and lexer.

What do you think?

(in the meantime we could merge this because it's already an improvement)

@RX14 RX14 added this to the Next milestone Jan 2, 2018
@RX14 RX14 merged commit 856220c into crystal-lang:master Jan 2, 2018
@makenowjust makenowjust deleted the fix/crystal/parse-regex-in-default-arg branch January 2, 2018 00:54
lukeasrodgers pushed a commit to lukeasrodgers/crystal that referenced this pull request Jan 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet

Successfully merging this pull request may close these issues.

None yet

3 participants