Skip to content

Commit

Permalink
Removed global variables. Fixes #3139
Browse files Browse the repository at this point in the history 
The Global AST node is still around, and code associated to it too,
because it's used internally by the compiler to store pre-compiled
regexes. The special $~ and $! are also represented as globals,
in the syntax. We can improve this in the future.
  • Loading branch information
Ary Borenszweig committed Aug 18, 2016
1 parent 773eeca commit e872c71
Show file tree
Hide file tree
Showing 38 changed files with 1,263 additions and 1,115 deletions.
45 changes: 36 additions & 9 deletions spec/compiler/codegen/block_spec.cr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -563,18 +563,27 @@ describe "Code gen: block" do
class Bar < Foo
end
$x : Int32?
class Global

This comment has been minimized.

Sign in to view

Copy link
@bcardiff

bcardiff Aug 19, 2016

Member

Maybe we should add a macro for class properties. WDYT?

class Global
   self_property x = 0
   c_property x = 0
   class_property x = 0

   property x = 0, on: self # (?) maybe we can reuse plain property macro.
end

This comment has been minimized.

Sign in to view

Copy link
@jhass

jhass Aug 19, 2016

Member

Definitely. Other possibilities:

module Global
  property self.x = 0
  property @@x = 0
end

I think I favor class_property still though.

This comment has been minimized.

Sign in to view

Copy link
@asterite

asterite Aug 19, 2016

Member

I don't know, I feel class properties are not that common and their usage is usually gated through some other methods/functionality. Let's find some real code that would benefit from these macros. While removing global variables in the std + examples I never needed this (I only needed it for some compiler specs, but these don't count as real code).

This comment has been minimized.

Sign in to view

Copy link
@Sija

Sija Aug 19, 2016

Contributor

Rails has mattr_accessor exactly for such purpose.
I'd vote 👍 for class_{property,getter,setter} macro.
@@x = 0
def self.x=(@@x)
end
def self.x
@@x
end
end
struct Int
def foo
x = Foo.new
x = Bar.new
x.do { $x = self }
x.do { Global.x = self }
end
end
123.foo
$x.to_i
Global.x.to_i
").to_i.should eq(123)
end

Expand Down Expand Up @@ -1220,40 +1229,58 @@ describe "Code gen: block" do

it "codegens block bug with conditional next and unconditional break (3)" do
run(%(
$x = 0
class Global
@@x = 0
def self.x=(@@x)
end
def self.x
@@x
end
end
def foo
a = 1234
a = yield 1
$x = a
Global.x = a
a
end
foo do |x|
next x if 1 == 1
break 0
end
$x
Global.x
)).to_i.should eq(1)
end

it "codegens block bug with conditional next and unconditional break (4)" do
run(%(
$x = 0
class Global
@@x = 0
def self.x=(@@x)
end
def self.x
@@x
end
end
def foo
bar(yield 1)
end
def bar(x)
$x = x
Global.x = x
end
foo do |x|
next x if 1 == 1
break 0
end
$x
Global.x
)).to_i.should eq(1)
end

Expand Down
4 changes: 1 addition & 3 deletions spec/compiler/codegen/case_spec.cr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,8 @@ describe "Code gen: case" do
run("
require \"prelude\"
$a = 0
def foo
$a += 1
1
end
case foo
Expand Down
16 changes: 12 additions & 4 deletions spec/compiler/codegen/class_spec.cr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -434,10 +434,14 @@ describe "Code gen: class" do
1
end
$x = self.foo.as(Int32)
@@x = self.foo.as(Int32)
def self.x
@@x
end
end
$x
Foo.x
)).to_i.should eq(1)
end

Expand All @@ -448,10 +452,14 @@ describe "Code gen: class" do
1
end
$x = self.as(Foo.class)
@@x = self.as(Foo.class)
def self.x
@@x
end
end
$x.foo
Foo.x.foo
)).to_i.should eq(1)
end

Expand Down
33 changes: 0 additions & 33 deletions spec/compiler/codegen/const_spec.cr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,14 +204,6 @@ describe "Codegen: const" do
").to_i.should eq(3)
end

it "works with const initialized after global variable" do
run(%(
$a = 1
COCO = $a
COCO
)).to_i.should eq(1)
end

it "works with variable declared inside if" do
run(%(
FOO = begin
Expand Down Expand Up @@ -327,31 +319,6 @@ describe "Codegen: const" do
)).to_i.should eq(3)
end

it "initializes const the moment it reaches it" do
run(%(
$x = 10
FOO = begin
a = $x
a
end
w = FOO
z = FOO
z
)).to_i.should eq(10)
end

it "initializes const when read" do
run(%(
$x = 10
z = FOO
FOO = begin
a = $x
a
end
z
)).to_i.should eq(10)
end

it "initializes simple const" do
run(%(
FOO = 10
Expand Down
17 changes: 13 additions & 4 deletions spec/compiler/codegen/double_splat_spec.cr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,19 +100,28 @@ describe "Codegen: double splat" do

it "evaluates double splat argument just once (#2677)" do
run(%(
$a = 0
class Global
@@x = 0
def self.x=(@@x)
end
def self.x
@@x
end
end
def data
$a += 1
{x: $a, y: $a, z: $a}
Global.x += 1
{x: Global.x, y: Global.x, z: Global.x}
end
def test(x, y, z)
end
test(**data)
$a
Global.x
)).to_i.should eq(1)
end
end
Loading

2 comments on commit e872c71

@luislavena
Copy link
Contributor

Choose a reason for hiding this comment

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

@asterite you mention:

The Global AST node is still around, and code associated to it too, because it's used internally by the compiler to store pre-compiled regexes.

Any hint on what would take to get rid of that particular usage by the compiler?

Also, what are your thoughts on deprecation of $~ and $? entirely?

Thank you.

@asterite
Copy link
Member

Choose a reason for hiding this comment

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

@luislavena This is the piece of code that uses globals internally. It could be changed to use class variables of a non-accessible module (with a name that can't be typed), or maybe at a more low-level way, more intrinsic to the compiler. I'll change it later, shouldn't be hard (I just wanted to push because later it's hard to rebase against master).

About $~ and $!: I don't know, I think they are pretty convenient. Consider this method, you would have to write it like described here. Maybe it doesn't matter much because doing a case over a regex might not be done all of the time.

If we remove them, we also need to remove backticks for process execution, for example:

output = `ls`
if $?.success?
  puts output
else
  puts "command failed"
end

or we'll have to make it return a tuple with the output and the status, or maybe another struct.

I think it's possible, I'm not sure they are that complex to understand and use. They will simplify the compiler's implementation, though. You can open an issue to discuss this, I'm open for a change :-)

Please sign in to comment.