Allow types other than Int in randstring
#54402
Conversation
Fixes JuliaLang#54397 What remains is that the effectively allowed input still needs to be convertible to `Int`. So things like `typemax(Int128)` won't work, and neither will negative values.
Seelengrab
added
the
domain:randomness
| @@ -71,13 +71,14 @@ let b = UInt8['0':'9';'A':'Z';'a':'z'] | |||
| global randstring | |||
|
|
|||
| function randstring(r::AbstractRNG, chars=b, n::Integer=8) | |||
| _n = convert(Int, n) | |||
There was a problem hiding this comment.
I am not sure it is worth doing in this specific instance but presumably structuring it like:
f(x::Integer) = f(convert(Int, n))
f(x::Int) = ...would reduce compilation time since you would not compile the main function body for multiple integer types.
There was a problem hiding this comment.
For cases where lots of different types are passed to randstring, that's certainly the case, yeah. I'm not sure how common that would be though, there's lots of better ways to control the length of a generated string other than through the type.
There was a problem hiding this comment.
I think @KristofferC is just referring to the amount of code that is generated by using function barriers or:
https://docs.julialang.org/en/v1/manual/performance-tips/#kernel-functions
There was a problem hiding this comment.
Yes, I'm referring to the likelihood of hitting that & the additional compilation overhead being a significant slowdown. If this is a bottleneck, I'd first recommend switching to a different scheme for generating the length (with just Int) than optimize the compilation overhead.
There was a problem hiding this comment.
Right, and the problem is that this PR would allow someone to have a type unstable call site with various Integer types for length, rather than error as it currently does.
There was a problem hiding this comment.
Why would that inherently be a problem? We don't prevent type instabilities in user code in other places either.
|
It is a problem because as this PR is currently written you will generate a
lot of redundant code. You should accept Kristoffer's suggestion for the
signatures as it will improve the quality of this PR.
…On Thu, May 9, 2024, 02:07 Sukera ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In stdlib/Random/src/misc.jl
<#54402 (comment)>:
> @@ -71,13 +71,14 @@ let b = UInt8['0':'9';'A':'Z';'a':'z']
global randstring
function randstring(r::AbstractRNG, chars=b, n::Integer=8)
+ _n = convert(Int, n)
Why would that inherently be a problem? We don't prevent type
instabilities in user code in other places either.
—
Reply to this email directly, view it on GitHub
<#54402 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAP5MDAUQXLRZEXJIYX554LZBMHALAVCNFSM6AAAAABHMSEKMSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANBXGIYTEMJQHA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
Since Kristoffer mentioned that it's probably not worth it in this PR, I've chosen to keep the code simpler to follow without having to disentangle niche dispatch/compilation optimizations. |
Fixes #54397
What remains is that the effectively allowed input still needs to be convertible to
Intand non-negative. So things liketypemax(Int128)won't work.