Conversation
TurkeyMan
requested review from
MartinNowak,
WalterBright,
ibuclaw and
schveiguy
as code owners
|
Thanks for your pull request and interest in making D better, @TurkeyMan! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. 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 fetch digger
dub run digger -- build "master + druntime#2819" |
| */ | ||
| bool casWeak(MemoryOrder succ = MemoryOrder.seq,MemoryOrder fail = MemoryOrder.seq,T,V)(T* here, T* ifThis, V writeThis) pure nothrow @nogc @trusted | ||
| if (!is(T == shared S, S) && !is(V == shared U, U)) | ||
| if (!is(T == shared S, S) && !is(V == shared U, U)) |
There was a problem hiding this comment.
Before your change this was the official DStyle (https://dlang.org/dstyle).
There was a problem hiding this comment.
It was inconsistent with the rest of the file... I would always favour internal consistency :/
There was a problem hiding this comment.
We have a style guide for a reason :/
|
Maybe refactor the one in |
|
Those spinlock backoff counters look sus; they're very small numbers. |
TurkeyMan
force-pushed
the
pause_function
branch
from
93c01d0 to
32e6634
Compare
|
|
Yeah, it's not a spin-lock... and it's also really bad >_< |
TurkeyMan
force-pushed
the
pause_function
branch
from
32e6634 to
7c3da5f
Compare
|
Is this controversial? |
| @@ -1,4 +1,4 @@ | |||
| Added overloads for `core.atomic.cas` which don't discard their result. | |||
| Added overloads for `core.atomic : cas` which don't discard their result | |||
There was a problem hiding this comment.
Unrelated style change
Ditto for all other changelogs and template constraints
| * loads and stores after the call. | ||
| */ | ||
| void atomicFence(MemoryOrder order = MemoryOrder.seq)() nothrow @nogc @safe | ||
| void atomicFence(MemoryOrder order = MemoryOrder.seq)() pure nothrow @nogc @safe |
There was a problem hiding this comment.
Unrelated to pause(), please move this into another PR
Ditto for all other pure additions
| import core.time; | ||
| if (k < pauseThresh) | ||
| return pause(); | ||
| return core.atomic.pause(); |
There was a problem hiding this comment.
Correct if im wrong but this is more than a simple refactoring as the new inline assembly differs:
Old:
rep
nop
New:
naked
rep
nop
ret
| ret; | ||
| } | ||
| } | ||
| else version (D_InlineAsm_X86_64) |
There was a problem hiding this comment.
Keep the old enum/version declaration instead of duplicating the assembly? Or should there be different implementations according to your comment regarding the pause opcode?
6 participants
So that other architectures can add their special sauce.