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.

Sign up for GitHub

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

Fix macro interpreter Number type of arithmetic expressions #5972

Merged
merged 4 commits into from
Jun 7, 2018
Merged

Fix macro interpreter Number type of arithmetic expressions #5972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3ad2864
Fix macro interpreter Number type of arithmetic expressions
straight-shoota Apr 20, 2018
4075f4e
Fix codegen error
straight-shoota Apr 22, 2018
cab5681
Fix StringBuilder#to_i
straight-shoota Apr 22, 2018
4f66593
Fix API usage added in #5954
straight-shoota Apr 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions spec/compiler/macro/macro_methods_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,10 @@ module Crystal
assert_macro "", "{{1 + 2}}", [] of ASTNode, "3"
end

it "executes + and preserves type" do
assert_macro "", "{{1_u64 + 2_u64}}", [] of ASTNode, "3_u64"
end

it "executes -" do
assert_macro "", "{{1 - 2}}", [] of ASTNode, "-1"
end
Expand Down
6 changes: 3 additions & 3 deletions spec/compiler/semantic/enum_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,9 +193,9 @@ describe "Semantic: enum" do
end
))
enum_type = result.program.types["Foo"].as(EnumType)
enum_type.types["OtherNone"].as(Const).value.should eq(NumberLiteral.new(0, :i32))
enum_type.types["Bar"].as(Const).value.should eq(NumberLiteral.new(1, :i32))
enum_type.types["Baz"].as(Const).value.should eq(NumberLiteral.new(2, :i32))
enum_type.types["OtherNone"].as(Const).value.should eq(NumberLiteral.new("0", :i32))
enum_type.types["Bar"].as(Const).value.should eq(NumberLiteral.new("1", :i32))
enum_type.types["Baz"].as(Const).value.should eq(NumberLiteral.new("2", :i32))
end

it "disallows None value when defined with @[Flags]" do
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/crystal/macros/methods.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -699,7 +699,7 @@ module Crystal
end

if value
NumberLiteral.new(value)
NumberLiteral.new(value.to_s, :i32)
Copy link
Contributor

Choose a reason for hiding this comment

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

a few line above the value is converted to an Int64 (@value.to_i64?), it feels weird to 'cast' it to :i32 here, is it intentional ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is intentional as far as this prevents codegen to fail. 🤷‍♂️ It's only a workaround, though.
In fact, this isn't even a change: it implicitly was :i32 before, now it's just explicit.

But yes, it should probably be adjusted, however we need to fix the codegen bug first.

else
raise "StringLiteral#to_i: #{@value} is not an integer"
end
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/crystal/semantic/bindings.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ module Crystal
if visitor
numeric_value = visitor.interpret_enum_value(value)
numeric_type = node_type.program.int?(numeric_value) || raise "BUG: expected integer type, not #{numeric_value.class}"
type_var = NumberLiteral.new(numeric_value, numeric_type.kind)
type_var = NumberLiteral.new(numeric_value.to_s, numeric_type.kind)
type_var.set_type_from(numeric_type, from)
else
node.raise "can't use constant #{node} (value = #{value}) as generic type argument, it must be a numeric constant"
Expand Down
4 changes: 2 additions & 2 deletions src/compiler/crystal/semantic/main_visitor.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2521,7 +2521,7 @@ module Crystal
# (useful for sizeof inside as a generic type argument, but also
# to make it easier for LLVM to optimize things)
if type && !node.exp.is_a?(TypeOf)
expanded = NumberLiteral.new(@program.size_of(type.sizeof_type))
expanded = NumberLiteral.new(@program.size_of(type.sizeof_type).to_s, :i32)
expanded.type = @program.int32
node.expanded = expanded
end
Expand All @@ -2546,7 +2546,7 @@ module Crystal
# (useful for sizeof inside as a generic type argument, but also
# to make it easier for LLVM to optimize things)
if type && type.instance_type.devirtualize.class? && !node.exp.is_a?(TypeOf)
expanded = NumberLiteral.new(@program.instance_size_of(type.sizeof_type))
expanded = NumberLiteral.new(@program.instance_size_of(type.sizeof_type).to_s, :i32)
expanded.type = @program.int32
node.expanded = expanded
end
Expand Down
6 changes: 3 additions & 3 deletions src/compiler/crystal/semantic/top_level_visitor.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,15 +576,15 @@ class Crystal::TopLevelVisitor < Crystal::SemanticVisitor
unless existed
if enum_type.flags?
unless enum_type.types["None"]?
none = NumberLiteral.new(0, enum_base_type.kind)
none = NumberLiteral.new("0", enum_base_type.kind)
none.type = enum_type
enum_type.add_constant Arg.new("None", default_value: none)

define_enum_none_question_method(enum_type, node)
end

unless enum_type.types["All"]?
all = NumberLiteral.new(all_value, enum_base_type.kind)
all = NumberLiteral.new(all_value.to_s, enum_base_type.kind)
all.type = enum_type
enum_type.add_constant Arg.new("All", default_value: all)
end
Expand Down Expand Up @@ -643,7 +643,7 @@ class Crystal::TopLevelVisitor < Crystal::SemanticVisitor
end

all_value |= counter
const_value = NumberLiteral.new(counter, base_type.kind)
const_value = NumberLiteral.new(counter.to_s, base_type.kind)
member.default_value = const_value
if enum_type.types.has_key?(member.name)
member.raise "enum '#{enum_type}' already contains a member named '#{member.name}'"
Expand Down
22 changes: 20 additions & 2 deletions src/compiler/crystal/syntax/ast.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,8 +214,8 @@ module Crystal
def initialize(@value : String, @kind = :i32)
end

def initialize(value : Number, @kind = :i32)
@value = value.to_s
def self.new(value : Number)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not continue to accept .new(Number, kind: Symbol), but have .new(Number) take the type of the number. This way you don't need the to_s additions above (they all have explicit kind anyway).

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this would be that useful. It's a simple API now: If the constructor receives a Number, it deduces kind accordingly. Otherwise you need to specify value (as string) and kind explicitly.

new(value.to_s, kind_from_number(value))
end

def has_sign?
Expand Down Expand Up @@ -243,6 +243,24 @@ module Crystal

def_equals value.to_f64, kind
def_hash value, kind

def self.kind_from_number(number : Number)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the only usage of kind_from_number? or there are plans to reuse it somewhere else?

Asking this because either make it protected/private or add :nodoc:.

Thank you.

Copy link
Member Author

Choose a reason for hiding this comment

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

Atm it is the only usage, I don't know if it there is a different use case.

Compiler types and methods are not really documented anyway, so there is not much gain from adding :nodoc:.

I don't think there are any particular guidelines about this for the compiler.

case number
when Int8 then :i8
when Int16 then :i16
when Int32 then :i32
when Int64 then :i64
when Int128 then :i128
when UInt8 then :u8
when UInt16 then :u16
when UInt32 then :u32
when UInt64 then :u64
when UInt128 then :u128
when Float32 then :f32
when Float64 then :f64
else raise "Unsupported Number type for NumberLiteral: #{number.class}"
end
end
end

# A char literal.
Expand Down