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

Avoid lazy initialization of class variables assigned with simple values #8067

Merged

Conversation

waj
Copy link
Member

@waj waj commented Aug 9, 2019

Currently all class variables are lazily initialized even when they are declared with a simple initializer with a literal of a simple type. This prevents that and do a direct initialization of the global variable, quite similar to simple constants.

I'm not so happy with this patch as I do with the result, so I'm open to suggestions about how to write this better.

Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

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

Looks good!

@asterite asterite added this to the 0.31.0 milestone Aug 12, 2019
@asterite
Copy link
Member

asterite commented Aug 12, 2019

@waj Maybe we can add a couple of specs for this:

  1. A spec that proves that inherited simple class variables are initialized
  2. A spec that checks that the "init" stuff doesn't appear for simple class variables. Something like this (but looking for the "init:..." thing we use in LLVM).

What do you think?

@waj
Copy link
Member Author

waj commented Aug 12, 2019

@asterite I just added a spec for 2. Isn't 1 already covered by this spec?

it "doesn't inherit class var value in subclass" do
run(%(
class Foo
@@var = 1
def self.var
@@var
end
def self.var=(@@var)
end
end
class Bar < Foo
end
Foo.var = 2
Bar.var
)).to_i.should eq(1)
end

@asterite
Copy link
Member

@waj You are right, just that spec is good, thanks!

@waj waj added the pr:on-hold-do-not-merge This PR is on hold. Do not merge. label Aug 12, 2019
@asterite asterite merged commit 3eda43d into crystal-lang:master Aug 12, 2019
@bcardiff bcardiff removed the pr:on-hold-do-not-merge This PR is on hold. Do not merge. label Sep 16, 2019
@bcardiff bcardiff changed the title Avoid lazy inizialization of class variables assigned with simple values Avoid lazy initialization of class variables assigned with simple values Sep 16, 2019
@waj waj deleted the feature/simple-class-var-initialization branch February 12, 2020 20:22
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