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

change widen to strict 2x widening #28045

Merged
merged 1 commit into from
Jul 11, 2018
Merged

change widen to strict 2x widening #28045

merged 1 commit into from
Jul 11, 2018

Conversation

sbromberger
Copy link
Contributor

@sbromberger sbromberger commented Jul 10, 2018

Per #14407 (comment) et seq., change the behavior of widen() as follows:

for all types with bit size n, widen() will produce a similarly-signed type with bit size 2n regardless of underlying word size / architecture. This changes the following:

Type Current widen() This PR
UInt8 UInt UInt16
UInt16 UInt UInt32
Int8 Int Int16
Int16 Int Int32

In addition, added explicit type tests.

@simonbyrne
Copy link
Contributor

Given the relatively small amount of code churn in the tests, I don't think this should be too breaking.

Copy link
Member

@StefanKarpinski StefanKarpinski left a comment

Choose a reason for hiding this comment

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

LGTM. It's a good sign that the only things that need to change seem to be explicit tests for the return type. Also, you gotta love how easy this kind of change is in Julia 😁

@simonbyrne
Copy link
Contributor

Maybe explain this in the docs?

@JeffBezanson
Copy link
Member

There's an item about widen in NEWS.md that should be changed to reflect this.

@ararslan ararslan added needs news A NEWS entry is required for this change needs docs Documentation for this change is required labels Jul 10, 2018
@sbromberger
Copy link
Contributor Author

I didn't know how to update NEWS without an issue/PR number. Should I include the NEWS.md changes as another commit here now that we know it's #28045 ?

@simonbyrne
Copy link
Contributor

Yes, it's fine to add as another commit. We can squash it all on merge.

@sbromberger
Copy link
Contributor Author

OK, what doc changes do people suggest? I think the current docstring is fairly clear. Do we really need to change it? (ref https://docs.julialang.org/en/latest/base/base/#Base.widen)

@StefanKarpinski
Copy link
Member

It is fairly clear and this is still in line with it. Might be good to explicitly state that for fixed-size integer types less than 128-bits widen doubles the bit size since that's now the rule.

@simonbyrne
Copy link
Contributor

Something like:

For fixed-width integer types, this will a type

@simonbyrne simonbyrne closed this Jul 10, 2018
@simonbyrne simonbyrne reopened this Jul 10, 2018
@simonbyrne
Copy link
Contributor

Sorry, I pressed the wrong button.

@sbromberger
Copy link
Contributor Author

OK, I made changes to NEWS.md and to the docstring in base/operators.jl and fixed up the commits.

@sbromberger
Copy link
Contributor Author

@StefanKarpinski

It's a good sign that the only things that need to change seem to be explicit tests for the return type.

I was actually puzzled as to why the return types weren't being tested in the first place (except for BigInt).

@ararslan ararslan removed needs docs Documentation for this change is required needs news A NEWS entry is required for this change labels Jul 11, 2018
@@ -778,7 +778,10 @@ fldmod1(x, y) = (fld1(x, y), mod1(x, y))

If `x` is a type, return a "larger" type, defined so that arithmetic operations
`+` and `-` are guaranteed not to overflow nor lose precision for any combination
of values that type `x` can hold.
of values that type `x` can hold.
Copy link
Member

Choose a reason for hiding this comment

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

Trailing whitespace on this line and the next.

@sbromberger
Copy link
Contributor Author

That's a weird test in numbers.jl. Why is that there?

@sbromberger
Copy link
Contributor Author

It doesn’t look like the appveyor failure is related. Could someone please confirm?

@martinholters
Copy link
Member

Yep, looks like a problem with httpbin.org which happens quite a bit lately.

@StefanKarpinski
Copy link
Member

The Win32 failure is indeed the httpbin flakiness and is safe to ignore. So this has passed on all platforms and is ready to merge as far as I can tell. I'll merge later today unless there are objections.

@StefanKarpinski StefanKarpinski merged commit ab5fd17 into JuliaLang:master Jul 11, 2018
@StefanKarpinski
Copy link
Member

Eh, why wait. Just went for it.

@sbromberger
Copy link
Contributor Author

Thank you all for the guidance! This was an easy PR.

@sbromberger sbromberger deleted the sbromberger/widen-2x branch July 11, 2018 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants