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

simplify rules for mixed-signedness integer arithmetic (#9292) #23811

Merged
merged 1 commit into from
Sep 22, 2017

Conversation

JeffBezanson
Copy link
Member

This implements "option 5", from #9292 (comment):

  • For different-size arguments, the larger type wins
  • Otherwise the unsigned type wins

- for different-size arguments, the larger type wins
- otherwise the unsigned type wins
@JeffBezanson JeffBezanson added this to the 1.0 milestone Sep 21, 2017
@JeffBezanson JeffBezanson added the breaking This change will break code label Sep 21, 2017
@StefanKarpinski
Copy link
Member

Oh wow, that was quick. Did you run the test suite locally? Were no code base changes required?

@JeffBezanson
Copy link
Member Author

Apparently tests pass and these cases just don't come up much (at least in Base).

@StefanKarpinski
Copy link
Member

Sure, but https://github.com/JuliaLang/julia/pull/22482/files had to make a bunch of incidental changes so zero changes is actually impressive. I also suspect that's a very good sign about this particular rule – i.e. that it's usually what you want. Nice!

@StefanKarpinski
Copy link
Member

Restarted the Circle CI x86_64 build but I suspect this is good. Further thoughts: the fact that this required zero changes suggests to me that people are already programming according to this rule intuitively; in cases where the old rules disagreed with the new one are cases where they were not sure what the rule was and probably wrote the code defensively with explicit conversions. I would further note that this doesn't just simplify the rule for mixed-signedness, it completely unifies the behavior for mixed or unmixed signedness (@JeffBezanson obviously already knows this since he wrote the tests in here that demonstrate the new, uniform behavior).

@StefanKarpinski
Copy link
Member

Same segfault again; unrelated to this PR.

@JeffBezanson
Copy link
Member Author

I noticed that after this, UInt32 is still a bit strange, in that it is the only type T such that (x::T) + 1 changes from Signed to Unsigned depending on platform (i.e. the type of 1). I don't know if there's any good way to plug that hole though. We could always return Int64, or always return UInt32, but neither of those is really satisfying. Or we could make UInt32 + Int64 = UInt64 on 64-bit. Not clear if this is really worth a special case, but something to think about if we're not sick of unsigned arithmetic issues yet :)

@StefanKarpinski
Copy link
Member

Options that I can think of:

  1. Make same-size mixed signedness integer promotion an error and require explicit casting.

  2. Promote same-size, mixed-signedness integers to a wider signed integer type. This makes result types the same, regardless of word size: we have UInt32 + Int => Int64 and UInt64 + Int => Int128 (different parts of the rules apply but the result is the same). This pushes the problem to Int32 + UInt => Int64 | UInt64 and Int64 + UInt => Int64 | Int128 both depending on platform size, but that's less of a problem since there are no unsigned literals whose size depends on platform word size.

  3. Similar to 2 but less dramatic: let the signed type win in same-size, mixed signedness integer promotions. This makes result signedness the same, although not the same type. Regardless of word size: we have UInt32 + Int => Int and UInt64 + Int => Int64. Again, this pushes the problem to Int32 + UInt => Int32 | UInt64 depending on platform size (pleasantly, Int64 + UInt => Int64 regardless of platform).

Option 3 seems best: just let signed integer types win in same-size, mixed signedness interactions. I think this is dictated by having platform-dependent signed literals but not unsigned ones.

@StefanKarpinski
Copy link
Member

Oops – I'm not sure what I was doing here, these are the actual promotion tables:

Unsigned wins (current):

  • UInt32 + Int(32) => UInt(32)
  • UInt64 + Int(32) => UInt64
  • UInt32 + Int(64) => Int(64)
  • UInt64 + Int(64) => UInt(64)

Signed wins:

  • UInt32 + Int(32) => Int(32)
  • UInt64 + Int(32) => UInt64
  • UInt32 + Int(64) => Int(64)
  • UInt64 + Int(64) => Int(64)

So either way there's one case where the promotion of some type (UInt32 or UInt64) with Int changes signedness based on the platform word size.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants