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

Support Atomic(T)#compare_and_set when T is a reference union #13565

Merged

Conversation

HertzDevil
Copy link
Contributor

Atomic(T)#compare_and_set was not working when T is a union of reference types, because it wrongly used the code path for nilable reference unions. This PR fixes that by removing legacy code that is only relevant before LLVM 3.9.

It also adds checks against T in the arithmetic methods to make sure T is an arithmetic type rather than a pointer. LLVM would produce module validation errors otherwise if Atomic doesn't catch those errors.

@HertzDevil HertzDevil added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:concurrency labels Jun 15, 2023
end

# Performs `atomic_value &+= value`. Returns the old value.
#
# `T` cannot contain any reference types.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest adding a NOTE admonition.

ditto elsewhere

Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate why? This seems like a very basic part of the description, rather than a callout.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a hard requirement, thus IMO deserves visibility.

Copy link
Member

Choose a reason for hiding this comment

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

But that's not exactly what NOTE admonitions are for.

Copy link
Member

Choose a reason for hiding this comment

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

It's hard to use admonitions or agree on what they should mean. I think NOTE: should be the same as a footnote, but it's hard to stick to this definition because the reality is that the current look increases visibility rather than decrease it.

Copy link
Contributor

@Sija Sija Jun 15, 2023

Choose a reason for hiding this comment

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

@straight-shoota Oh, I didn't know they're defined (not in the docs they aren't at least) ;)

@HertzDevil
Copy link
Contributor Author

It turns out LLVM doesn't support pointers in #swap until 15.0: llvm/llvm-project@18e6b82

@beta-ziliani
Copy link
Member

Would it be sensible to still add support for it for LLVM >= 15?

@HertzDevil
Copy link
Contributor Author

We just need to bring back the existing pointerof(@value).as(LibC::SizeT*) workaround in #swap. I am more surprised by all the years it took them

@straight-shoota straight-shoota added this to the 1.9.0 milestone Jun 23, 2023
@straight-shoota straight-shoota merged commit d2add86 into crystal-lang:master Jun 24, 2023
Blacksmoke16 pushed a commit to Blacksmoke16/crystal that referenced this pull request Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:concurrency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants