Skip to content

Refactor symbols for primitive number kinds to enums#11621

Merged
straight-shoota merged 4 commits intocrystal-lang:masterfrom
HertzDevil:refactor/number-kind-enum
Feb 3, 2022
Merged

Refactor symbols for primitive number kinds to enums#11621
straight-shoota merged 4 commits intocrystal-lang:masterfrom
HertzDevil:refactor/number-kind-enum

Conversation

@HertzDevil
Copy link
Contributor

A few of the case expressions could be further made exhaustive once #11576 is merged.

Comment on lines +30 to 39
when .i8? then FFI::Type.sint8
when .u8? then FFI::Type.uint8
when .i16? then FFI::Type.sint16
when .u16? then FFI::Type.uint16
when .i32? then FFI::Type.sint32
when .u32? then FFI::Type.uint32
when .i64? then FFI::Type.sint64
when .u64? then FFI::Type.uint64
else
raise "BUG: missing ffi_type for #{self} (#{self.class})"
Copy link
Member

Choose a reason for hiding this comment

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

I suppose this could already be exhaustive with replacing else by an explicit in .i128?, .u128? branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not true, there are also .f32? and .f64? (and the opposite in Crystal::FloatType#ffi_type would need all 10 integer checks)

Comment on lines +1248 to +1255
# interpreter-exclusive integer unions
private enum MixedNumberKind
# Int64 | UInt64
Mixed64

# Int128 | UInt128
Mixed128
end
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering about this type... could this be a sign that NumberKind should actually be a flag enum so that we could express I64 | U64 directly?

Probably not. But it nevertheless it feels odd to have such an explicit type to represent a simple internal message. It's just used in two methods and we don't need a strong formalization for this.
The code that handles these mixed types could be refactored to work without an extra type. It would be sufficient to have extend_int return some value (like nil - or the bitsize value as an int) to indicate mixed. The handlers works with the type ranks for further branching, so they could even figure out whether it's 64 or 128 bits if it isn't told.
Delaying branching over bitsize could even lead to some simplification in the branches that handle mixed types, especially in primitive_binary_op_cmp. That's unrelated though.
Such a refactoring could be done prior to this PR.

To illustrate, this would be the diff for a very crude refactoring to return the raw bitsize for mixed type:

--- a/src/compiler/crystal/interpreter/primitives.cr
+++ b/src/compiler/crystal/interpreter/primitives.cr
@@ -773,7 +773,7 @@ class Crystal::Repl::Compiler
   private def primitive_binary_op_math(left_type : IntegerType, right_type : IntegerType, left_node : ASTNode?, right_node : ASTNode, node : ASTNode, op : String)
     kind = extend_int(left_type, right_type, left_node, right_node, node)
     case kind
-    when :mixed_64
+    when 64
       if left_type.rank > right_type.rank
         # It's UInt64 op X where X is a signed integer
         left_node ? left_node.accept(self) : put_self(node: node)
@@ -829,7 +829,7 @@ class Crystal::Repl::Compiler

         kind = :i64
       end
-    when :mixed_128
+    when 128
       if left_type.rank > right_type.rank
         # It's UInt128 op X where X is a signed integer
         left_node ? left_node.accept(self) : put_self(node: node)
@@ -885,11 +885,13 @@ class Crystal::Repl::Compiler

         kind = :i128
       end
-    else
+    when Symbol
       # Go on
       return false unless @wants_value

       primitive_binary_op_math(node, kind, op)
+    else
+      raise "BUG"
     end

     if kind != left_type.kind
@@ -1113,7 +1115,7 @@ class Crystal::Repl::Compiler
   private def primitive_binary_op_cmp(left_type : IntegerType, right_type : IntegerType, left_node : ASTNode, right_node : ASTNode, node : ASTNode, op : String)
     kind = extend_int(left_type, right_type, left_node, right_node, node)
     case kind
-    when :mixed_64
+    when 64
       if left_type.rank > right_type.rank
         # It's UInt64 == X where X is a signed integer.

@@ -1136,7 +1138,7 @@ class Crystal::Repl::Compiler

         cmp_i64_u64(node: node)
       end
-    when :mixed_128
+    when 128
       if left_type.rank > right_type.rank
         # It's UInt128 == X where X is a signed integer.

@@ -1283,7 +1285,7 @@ class Crystal::Repl::Compiler

       :i64
     elsif left_type.rank <= 8 && right_type.rank <= 8
-      :mixed_64
+      64
     elsif left_type.rank <= 9 && right_type.rank <= 9
       # If both fit in an Int128
       # Convert them to Int128 first, then do the comparison
@@ -1295,7 +1297,7 @@ class Crystal::Repl::Compiler

       :i128
     else
-      :mixed_128
+      128
     end
   end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

64 and 128 are hardcoded constants that bear no numerical significance here. This is exactly why they should be represented as an enum.

@oprypin oprypin self-requested a review January 27, 2022 22:42
@straight-shoota straight-shoota added this to the 1.4.0 milestone Feb 2, 2022
@straight-shoota straight-shoota changed the title Use enums instead of symbols for primitive number kinds Refactor symbols for primitive number kinds to enums Feb 3, 2022
@straight-shoota straight-shoota merged commit 58e8fc5 into crystal-lang:master Feb 3, 2022
@HertzDevil HertzDevil deleted the refactor/number-kind-enum branch February 5, 2022 01:50
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