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

Compiler: fix crash when formatting syntax error inside macro #8055

Merged
merged 1 commit into from
Aug 12, 2019

Conversation

asterite
Copy link
Member

@asterite asterite commented Aug 6, 2019

Fixes #8038

Also improves the code a bit.

💭 I wonder whether we should make min, max, etc., always be nilable... that way you would always remember to handle the empty case, and in case you are sure you aren't dealing with an empty collection you can always stick a not_nil! there.

@asterite asterite added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler labels Aug 6, 2019
@bcardiff
Copy link
Member

bcardiff commented Aug 6, 2019

Given that an enumerable can be empty as a value, it could make sense to min/max be nilable, and have min!/max! as an unsafe “trust-me-is-not-empty” version. But on other hand the operation itself is not defined for empty sets, so is reasonable to have a non trivial precondition.

@oprypin
Copy link
Member

oprypin commented Aug 7, 2019

I wonder whether we should make min, max, etc., always be nilable

That's the kind of thing that causes a knee-jerk negative reaction for me.
But then if I actually think about it, it totally is an improvement.

always remember to handle the empty case

@bcardiff bcardiff added this to the 0.30.1 milestone Aug 8, 2019
@bcardiff
Copy link
Member

bcardiff commented Aug 9, 2019

The linux32 CI is failing constantly this time. Maybe #8065 could help

spaces = match[1]?
return 0 unless spaces

spaces.size
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this part is refactored, we can simplify it by avoiding using a regex:

line.each_char_with_index do |char, index|
  return index if char != ' '
end
0

Copy link
Member Author

Choose a reason for hiding this comment

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

No, \s matches tab, spaces, maybe also unicode space, I don't know.

This code runs once when the compiler finds an error, optimizing here has no impact at all. Maybe it's 5ms faster, the user can't possibly tell.

I also prefer avoiding changing code if it's not broken.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ho, there is char.ascii_whitespace?

But that's basically what's done here, the code is already changed.

@bcardiff bcardiff merged commit 4a0ca22 into crystal-lang:master Aug 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unhandled compiler exception: Empty enumerable (Enumerable::EmptyError)
4 participants