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

Add MetaVar#defualt_value and MetaVar#has_default_value? #5974

Merged
merged 1 commit into from
Apr 20, 2018
Merged

Add MetaVar#defualt_value and MetaVar#has_default_value? #5974

merged 1 commit into from
Apr 20, 2018

Conversation

asterite
Copy link
Member

@asterite asterite commented Apr 20, 2018

Fixes #4387

After this, my idea is to implement some form of meta attribute to be able to implement JSON/YAML/others serialization in a much better, extensible way. So this is just the first step towards that, because one needs to know whether an instance variable has a default value, so if it's not present in the document, when mapping, one should not error.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@asterite asterite self-assigned this Apr 20, 2018
@asterite
Copy link
Member Author

Small note: I didn't want to make MetaVar bigger by adding a default_value property, because it's used a lot in the semantic pass, so I created a MetaMacroVar class to replace the old one, just for macros.

@asterite asterite merged commit 27faaee into crystal-lang:master Apr 20, 2018
@RX14 RX14 added this to the Next milestone Apr 20, 2018
@straight-shoota
Copy link
Member

@asterite Do default_value and has_default_value? return appropriate results for default values nil and false as well as uninitialized ivars? Judging from the code I suspect that at least false would be incorrectly reported as no default value.

end

def class_desc
"MetaVar"
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this return "MetaMacroVar"?

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, for the user this is the old MetaVar

@asterite
Copy link
Member Author

@straight-shoota that's the reason why has_default_value? exists. Otherwise, compile the compiler and try it out.

An uninitialized variable doesn't have a default value.

@Sija
Copy link
Contributor

Sija commented Apr 21, 2018

@asterite Wouldn't var initialized with false value give false positive (pun intended ;)) in:

when "has_default_value?"
interpret_argless_method(method, args) do
BoolLiteral.new(!!default_value)
end
?

@asterite
Copy link
Member Author

default_value's type is ASTNode?. It's either nil (no default value) or ASTNode (has a default value, which can be NilLiteral, BoolLiteral, etc.). It's fine.

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

4 participants