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

atomics: switch to using Pair for return pairs #41659

Merged
merged 1 commit into from
Aug 10, 2021
Merged

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Jul 20, 2021

This makes many more functions type-stable, by directly preserving the
element type when making the copy, and prints as old => new, like the
argument to atomic replace.

@vtjnash vtjnash added multithreading Base.Threads and related functionality backport 1.7 labels Jul 20, 2021
@JeffBezanson
Copy link
Member

I'm not totally sold on this; the case of @atomicreplace undermines it a bit since the old value and success flag are really just separate return values, not related as a replacement mapping or key-value pair.

@vtjnash
Copy link
Member Author

vtjnash commented Jul 21, 2021

That particular usage of pair is a common API pattern in C++ to return old => success, e.g. in https://en.cppreference.com/w/cpp/container/map/insert 😂

@JeffBezanson
Copy link
Member

That's a good example. In C++ pair is basically the same as a 2-tuple, though. I guess they have tuples now, but pair predates them by a lot? It looks to me like they'd use 2-tuples now if they were doing it over.

@JeffBezanson
Copy link
Member

Or, if we want something invariant, another possibility is to use namedtuples, and be more self-documenting at the same time.

This makes many more functions type-stable, by directly preserving the
element type when making the copy, and prints as `old => new`, like the
argument to atomic replace.

For replace, we use Pair{FT, FT} (like the argument). For modify, we use
NamedTuple{(:old, :success), Tuple{FT, Bool}}.
@vtjnash vtjnash merged commit cc3fc0b into master Aug 10, 2021
@vtjnash vtjnash deleted the jn/atomic-return-pair branch August 10, 2021 21:31
Comment on lines +819 to +822
# if we didn't inline this, it's probably because the callsite was actually dynamic
# to avoid potentially compiling many copies of this, we mark the arguments with `@nospecialize`
# but also mark the whole function with `@inline` to ensure we will inline it whenever possible
# (even if `convert(::Type{A}, a::A)` for some reason was expensive)
Copy link
Member

Choose a reason for hiding this comment

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

Should this comment have been moved to Base.jl? Or which method does it belong to?

KristofferC pushed a commit that referenced this pull request Aug 25, 2021
This makes many more functions type-stable, by directly preserving the
element type when making the copy, and prints as `old => new`, like the
argument to atomic replace.

For replace, we use Pair{FT, FT} (like the argument). For modify, we use
NamedTuple{(:old, :success), Tuple{FT, Bool}}.

(cherry picked from commit cc3fc0b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multithreading Base.Threads and related functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants