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

Do not use globals for regex #10951

Merged
merged 2 commits into from
Jul 24, 2021
Merged

Conversation

asterite
Copy link
Member

@asterite asterite commented Jul 15, 2021

This is mainly a compiler refactor.

When you use a regex literal:

/hello/

what the compiler did, before this PR, is something like this:

if (temp = $global)
  temp
else
  $global = Regex.new("hello")
end

YES, it was using global variables, which we removed from the language a long time ago. But the code around them was still kept because it was used for regex.

This is all fine, except that if, say, someone wants to write an interpreter for crystal ( 😉 ), they will have to implement global variables just for this.

This PR changes that logic to use a constant instead. So now it's more like this:

GLOBAL

Suspicious... where is GLOBAL declared? It's done internally as part of expanding the regex literal, there's no visible assignment to it. But it works!

An extra benefit is that the old check wasn't fiber/thread safe. Reading a constant is fiber/thread safe, so this isn't just a refactor to make implementing the interpreter easier to do.

With that, all the logic related to global variables is removed from the compiler. There was an AST node named Global which was still used for the special variables $~ and $?: this PR renames this node to SpecialVar.

@HertzDevil
Copy link
Contributor

Does renaming an AST node type name not constitute a breaking change to the macro language?

@asterite
Copy link
Member Author

Does renaming an AST node type name not constitute a breaking change to the macro language?

Oh, I guess so... Hmm... I'll just undo the last commit then.

@asterite asterite force-pushed the do-not-use-globals-for-regex branch from 1ba8085 to 5d34b3e Compare July 15, 2021 14:20
Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

A PR with lots of code removals that still brings actual improvements is really lovely.

@asterite
Copy link
Member Author

Do we know why CI fails? It seems to be failing in a lot of PRs...

@straight-shoota
Copy link
Member

I don't know what's wrong with aarch64 specs. But it doesn't seem to be related to this patch.

@straight-shoota straight-shoota added this to the 1.2.0 milestone Jul 19, 2021
@straight-shoota straight-shoota merged commit 4a54929 into master Jul 24, 2021
@straight-shoota straight-shoota deleted the do-not-use-globals-for-regex branch July 24, 2021 17:52
@HertzDevil
Copy link
Contributor

Before and after, prepared using a local instance of Compiler Explorer: https://gist.github.com/HertzDevil/182951aa615da448752729d71a45705d

@asterite
Copy link
Member Author

Do you think it's better or worse? I guess worse because of the atomic initialization, right?

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.

3 participants