-
-
Notifications
You must be signed in to change notification settings - Fork 10
Add get
with default
#26
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
base: main
Are you sure you want to change the base?
Conversation
src/ScopedValues.jl
Outdated
function get(val::ScopedValue{T}, default::T) where {T} | ||
scope = current_scope()::Union{Nothing, Scope} | ||
scope === nothing && return default | ||
scope = scope::Scope | ||
return Base.get(scope.values, val, default) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ::Union{Nothing, Scope}
is likely unecessary (and might even be slower).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ::Union{Nothing, Scope}
is how this is called upstream. I've removed it in this PR, but which is right?
|
Without requiring the ScopedValue to have a Union type.
aa724f3
to
28ca05a
Compare
Done.
Added a test. The benchmark does not look current for 1.12 (it uses
Cc: @KristofferC and @topolarity to see if there is anything objectionable to adding this.
How would you use the default in the scoped value in this case? |
That should be returned instead of the provided default. The default value is the current value, and get with default should only return the user-provided default if there is no current value. |
b543e78
to
6026aab
Compare
Per this comment.
Our hope, as reflected in one of the added tests, is that this will be non-allocating (unlike the basic
get
).