Skip to content

Conversation

kpamnany
Copy link
Member

@kpamnany kpamnany commented Aug 18, 2025

Per this comment.

Our hope, as reflected in one of the added tests, is that this will be non-allocating (unlike the basic get).

Comment on lines 261 to 266
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
Copy link
Member

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).

Copy link
Member Author

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?

@vchuravy
Copy link
Member

  1. Can you change the title to reflect that this is get with default? Non-allocating is an implementation detail that may or may not be true...
  2. Tests are missing, including addition to the benchmarks
  3. Likely needs a discussion with JuliaLang/Julia first since we are adding a new API here. E.g. we will want to upstream this eventually.
  4. I am not a fan of dropping the default value from the scoped value.

Without requiring the ScopedValue to have a Union type.
@kpamnany kpamnany changed the title Add non-allocating get Add get with default Aug 19, 2025
@kpamnany kpamnany force-pushed the kp-add-get-default branch from aa724f3 to 28ca05a Compare August 19, 2025 21:18
@kpamnany
Copy link
Member Author

  1. Can you change the title to reflect that this is get with default? Non-allocating is an implementation detail that may or may not be true...

Done.

2. Tests are missing, including addition to the benchmarks

Added a test. The benchmark does not look current for 1.12 (it uses logstate). It is also using scoped. But most importantly, I need to go read the documentation and understand the framework before I can add to it. I'll try and get to that later this week.

3. Likely needs a discussion with JuliaLang/Julia first since we are adding a new API here. E.g. we will want to upstream this eventually.

Cc: @KristofferC and @topolarity to see if there is anything objectionable to adding this.

4. I am not a fan of dropping the default value from the scoped value.

How would you use the default in the scoped value in this case?

@vchuravy
Copy link
Member

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.

@kpamnany kpamnany force-pushed the kp-add-get-default branch from b543e78 to 6026aab Compare August 20, 2025 15:27
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.

2 participants