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

Raise syntax error when using out as an arg #6052

Merged
merged 1 commit into from May 2, 2018

Conversation

esse
Copy link
Contributor

@esse esse commented May 2, 2018

That commit fixes issue #6042.

@bew
Copy link
Contributor

bew commented May 2, 2018

Can you add a spec somehow?

@Sija
Copy link
Contributor

Sija commented May 2, 2018

@bew it's there, see the diff.

@asterite asterite merged commit 25f8c25 into crystal-lang:master May 2, 2018
@straight-shoota
Copy link
Member

I'm not sure this is right. There should not really be an issue with accessing out as local variable. #6042 could be fixed differently.

@straight-shoota
Copy link
Member

I'm pretty sure @makenowjust didn't just forget to add a restriction to out in #5930.

@asterite
Copy link
Member

asterite commented May 2, 2018

out as a local variable should be forbidden. out is a keyword and it can be confused with the out var syntax.

@asterite
Copy link
Member

asterite commented May 2, 2018

That said, out taken as a keyword just for interacting with C seems like a bad design choice.

@asterite
Copy link
Member

asterite commented May 2, 2018

Basically...

out = 1
foo(out) # Error: expecting variable or instance variable after out

That's why out should not work for local variables, because you can't pass them to other methods.

@straight-shoota
Copy link
Member

Then all keywords should be forbidden?

out var could be confused with a method call, but not really with a local variable called out.

@straight-shoota
Copy link
Member

If out is not followed by a variable name, it's not a keyword.

@asterite
Copy link
Member

asterite commented May 2, 2018

@straight-shoota That's just confusing, but if someone wants to implement that, please go ahead (and revert this PR).

@makenowjust
Copy link
Contributor

@straight-shoota Yes, I did't forget to add out. However, @asterite's opinion makes a sense to me. I have no disagreement with this PR.

@RX14 RX14 added this to the Next milestone May 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants