Improve core.atomic.cas documentation, formatting, and constraints#3094
Improve core.atomic.cas documentation, formatting, and constraints#3094Geod24 wants to merge 3 commits intodlang:masterfrom
core.atomic.cas documentation, formatting, and constraints#3094Conversation
Due to the lack of recognition for immutable, it wasn't possible to do certain atomic operations, e.g. storing an immutable pointer to a struct into a shared pointer to an immutable struct.
In the case of `cas`, the first argument is a lvalue, while the other two can be rvalue. The template constraint was checking whether the lvalue argument type could be coerced to the rvalue argument type, however that is backward compared to how things are usually done in the language. We want rvalue to coerce to rvalue, never the other way around.
Geod24
requested review from
MartinNowak,
WalterBright,
ibuclaw and
schveiguy
as code owners
|
Thanks for your pull request, @Geod24! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + druntime#3094" |
|
I'm going to try another approach (a prior refactoring) |
| */ | ||
| pure nothrow @safe unittest | ||
| { | ||
| // A structure that is not atomically initialization (e.g. open files) |
| */ | ||
| bool cas(MemoryOrder succ = MemoryOrder.seq,MemoryOrder fail = MemoryOrder.seq,T,V1,V2)(T* here, V1 ifThis, V2 writeThis) pure nothrow @nogc @trusted | ||
| if (!is(T == shared) && is(T : V1)) | ||
| bool cas |
There was a problem hiding this comment.
Yeah I don't like it either, hence why I'm experimenting with: #3095
| bool cas | ||
| (MemoryOrder succ = MemoryOrder.seq, MemoryOrder fail = MemoryOrder.seq, T, V1, V2) | ||
| (T* here, V1 ifThis, V2 writeThis) pure nothrow @nogc @trusted | ||
| if (!is(T == shared) && is(V1 : T)) |
There was a problem hiding this comment.
hard to tell from the diff if you made functional changes or just reformat the existing code...
i'm having to audit this carefully, but i probably don't need to?
if this sort of refactor is fair game, i'll definitely just change this right back next time i touch it ;)
| bool cas | ||
| (MemoryOrder succ = MemoryOrder.seq, MemoryOrder fail = MemoryOrder.seq, T, V1, V2) | ||
| (shared(T)* here, V1 ifThis, V2 writeThis) pure nothrow @nogc @trusted | ||
| if (!is(T == class) && (is(V1 : T) || is(V1 : shared T))) |
There was a problem hiding this comment.
oh I see there is hidden changes!
what's the logic behind turning the type conversion around?
why is it interesting if here is implicitly convertible to ifThis? here is what's being mutated...
There was a problem hiding this comment.
You should review commit-by-commit, that will be much easier to see, and you will also see the rationale for it ;)
There was a problem hiding this comment.
Oh I'm sorry! I somehow looked at the inverse of what you're doing.
This is bizarre. Is it my mistake? Weird bug! O_O
So, I wonder if it's actually necessary for V1 to participate in the constraint?
It also seems odd for V1 to participate in the constraint but not V2...
I wonder if this is(V1 : T) || is(V1 : shared T) could be something like is(RemoveShared!V1 : T) ?
|
Blocked by #3095 |
|
#3095 is merged |
5 participants
Trying to get a lazily initialized cache working in
rt.backtrace.dwarf, of course things had to blow up somewhere else :)The first commit (slightly) correct the definition of
hasUnsharedIndirectionsto allow forimmutable(which is used in the second commit). Ultimately, we really should clean up this code duplication.The second commit contain a
unittestwith the code I was actually attempting to write, as well as the fix to make it pass. The last commit contains a slight rewrite of the documentation and formatting.CC @TurkeyMan