Fix edge cases where inexact conversions to UInt don't throw #51095
Conversation
LilithHafner
added
kind:bugfix
| @@ -939,7 +939,7 @@ for Ti in (Int8, Int16, Int32, Int64, Int128, UInt8, UInt16, UInt32, UInt64, UIn | |||
| end | |||
| end | |||
| function (::Type{$Ti})(x::$Tf) | |||
| if ($(Tf(typemin(Ti))) <= x <= $(Tf(typemax(Ti)))) && isinteger(x) | |||
| if ($(Tf(typemin(Ti))) <= x < $(Tf(typemax(Ti))+one(Tf))) && isinteger(x) | |||
There was a problem hiding this comment.
I think this deserves a source comment about why this isn't equivalent to <=.
There was a problem hiding this comment.
Seems quite clever, but I am slightly concerned about corner cases, since this is generic code. For example, if there happens to be a combination of integer and floating point types, with eps(Tf(typemax(Ti))) == 2.0, I think this might still have the issue of potentially giving the wrong answer. Can we instead just directly compute whether Tf(typemax(Ti)) is larger than typemax(Ti) and if so, use prevfloat(Tf(typemax(Ti))) here?
|
If
Technically, this is not generic code, it's metaprogramming to create methods with concrete type signatures; julia> for Ti in Base.BitInteger_types, Tf in (Float16, Float32, Float64)
if eps(Tf(typemax(Ti))) == 2.0
println((Ti, Tf))
end
end
julia> |
|
LLVM also now has a well-behaved intrinsic that may not require these complications anymore to avoid these problems and the added cost of working around the undefined behavior. Would this be helpful? https://llvm.org/docs/LangRef.html#llvm-fptoui-sat-intrinsic |
|
Our semantics here are to throw, so the saturating version would not help. |
|
I plan to go ahead and merge this when tests pass because it is I'm still happy to receive comments and perform revisions in a followup. |
LilithHafner
added
the
status:merge me
Good enough for me ;) |
LilithHafner
removed
the
status:merge me
LilithHafner
added
the
backport 1.10
(cherry picked from commit fb76136)
Backported PRs: - [x] #51095 <!-- Fix edge cases where inexact conversions to UInt don't throw --> - [x] #52583 <!-- Don't access parent of triangular matrix in powm --> - [x] #52645 <!-- update --gcthreads section in command line options --> - [x] #52423 <!-- update nthreads info in versioninfo --> - [x] #52721 <!-- inference: Guard TypeVar special case against vararg --> - [x] #52637 <!-- fix finding bundled stdlibs even if they are e.g. devved in an environment higher in the load path --> - [x] #52752 <!-- staticdata: handle cycles in datatypes --> - [x] #52758 <!-- use a Dict instead of an IdDict for caching of the `cwstring` for Windows env variables --> - [x] #51375 <!-- Insert hardcoded backlinks to stdlib doc pages --> - [x] #52994 <!-- place work-stealing queue indices on different cache lines to avoid false-sharing --> - [x] #53015 <!-- Add type assertion in iterate for logicalindex --> - [x] #53032 <!-- Fix a list in GC devdocs --> - [x] #52748 - [x] #52856 - [x] #52878 - [x] #52754 - [x] #52228 - [x] #52924 - [x] #52569 <!-- Fix GC rooting during rehashing of iddict --> - [x] #52605 <!-- Default uplo in symmetric/hermitian --> - [x] #52618 <!-- heap snapshot: add gc roots and gc finalist roots to fix unrooted nodes --> - [x] #52781 <!-- fix type-stability bugs in Ryu code --> - [x] #53055 <!-- Profile: use full terminal cols to show function name --> - [x] #53096 - [x] #53076 - [x] #52841 <!-- Extensions: make loading of extensions independent of what packages are in the sysimage --> - [x] #52078 <!-- Replace `⇔` by `↔` in documentation --> - [x] #53035 <!-- use proper cache-line size variable in work-stealing queue --> - [x] #53066 <!-- doc: replace harr HTML entity by unicode --> - [x] #52996 <!-- Apple silicon has 128 byte alignment so fix our defines to match --> - [x] #53121 Non-merged PRs with backport label: - [ ] #52694 <!-- Reinstate similar for AbstractQ for backward compatibility --> - [ ] #51479 <!-- prevent code loading from lookin in the versioned environment when building Julia -->
KristofferC
removed
the
backport 1.10
Fixes #51063