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

Preserve integer sizes in NumberLiteral#int_bin_op #10713

Merged
merged 3 commits into from
Jun 13, 2021

Conversation

collidedscope
Copy link
Contributor

Presently, the following macro raises a very surprising Error: Arithmetic overflow:

{{ 3000000000u64 % 2 }}

This is because NumberLiteral#int_bin_op converts the operands to Int32s via #to_i, but this unnecessarily and incorrectly discards information about the kinds of the operands. Since by this point we've eliminated the possibility that either of the operands are floats, we can safely cast them to Int in order to preserve their sizes.

@jhass
Copy link
Member

jhass commented May 15, 2021

Good change! Can you look into adding a spec?

@straight-shoota
Copy link
Member

Perhaps we could refactor the entire method to better utilize type safety?

    def int_bin_op(op, args)
      NumberLiteral.new(bin_op(op, args) do |me, other|
        if me.is_a?(Int) && other.is_a?(Int)
          yield me, other
        elsif me.is_a?(Float)
          raise "undefined method '#{op}' for float literal: #{self}"
        else
          raise "argument to NumberLiteral##{op} can't be float literal: #{self}"
        end
      end)
    end

@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:lang:macros labels May 15, 2021
@straight-shoota
Copy link
Member

I've added a spec and refactored the implementation of the method.

@straight-shoota straight-shoota added this to the 1.1.0 milestone Jun 10, 2021
@straight-shoota straight-shoota merged commit 6103d5a into crystal-lang:master Jun 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:lang:macros
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants