Fix Atomic#swap with reference types #6428
Conversation
|
Do all the other operations work? |
|
|
|
I guess they should check for references and error out with a nicer error message (use macro |
| @@ -157,7 +157,12 @@ struct Atomic(T) | |||
| # atomic.get # => 10 | |||
| # ``` | |||
| def swap(value : T) | |||
| Ops.atomicrmw(:xchg, pointerof(@value), value, :sequentially_consistent, false) | |||
| {% if T.union? && T.union_types.all? { |t| t == Nil || t < Reference } || T < Reference %} | |||
There was a problem hiding this comment.
Look at the compare_and_set method above for how to implement this. It uses different cases for nillable references and references. Currently this wont work for nilable references.
There was a problem hiding this comment.
It does work and I added a test specifically for that case. The difference from compare_and_set has to do with the return value of the method, not with the type of T.
There was a problem hiding this comment.
Oh, my bad.
It looks to me then like compare_and_set should work without the special case for nilables as Pointer(Void).null.as(String?) == nil.
There was a problem hiding this comment.
I tried what you suggested and it appears to work but perhaps whoever implemented it knows of some edge case we do not.
Anyway, that is a matter for another issue/PR.
There was a problem hiding this comment.
@Exilor yup, which is why I approved this :)
|
They probably should but that's outside the scope of this PR. |
Previously the compiler crashed with "Module validation failed: atomicrmw operand must have integer type!".
(followed the example of
#compare_and_set)