Conversation
JackStouffer
force-pushed
the
concurrency
branch
2 times, most recently
from
d459865 to
791df1a
Compare
|
Small request: also search for |
JackStouffer
force-pushed
the
concurrency
branch
from
791df1a to
a509c1a
Compare
|
@Hackerpilot Done |
JackStouffer
force-pushed
the
concurrency
branch
5 times, most recently
from
cb839be to
30b62b5
Compare
wilzbach
left a comment
wilzbach
left a comment
There was a problem hiding this comment.
First of all: thanks a lot for doing this and bringing the old module into "Phobos" style!
I found a few of tricky cases for which I wasn't sure if there is really a defined style, but apart from that this LGTM.
std/concurrency.d
Outdated
| { | ||
| scope(exit) notified = false; | ||
| scope (exit) | ||
| notified = false; |
There was a problem hiding this comment.
I am not sure whether this is defined by the style guide
| auto waiter = new Fiber({ receive(cond, received); }), notifier = new Fiber({ | ||
| send(cond, sent); | ||
| }); | ||
| waiter.call(); |
There was a problem hiding this comment.
This looks a bit weird as newlines are used only for the second Fiber expression
|
|
||
| auto r = new Generator!int( | ||
| { | ||
| auto r = new Generator!int({ |
There was a problem hiding this comment.
Again this isn't defined by the style guide
There was a problem hiding this comment.
It's consistent with then ending });, so I'm for it
JackStouffer
force-pushed
the
concurrency
branch
from
30b62b5 to
6120d6b
Compare
|
@wilzbach Fixed |
wilzbach
left a comment
wilzbach
left a comment
There was a problem hiding this comment.
I finally found a bit of time to make another pass through this. I found four small points worth discussing in terms of general Phobos style guidelines as there are many precedents for both variants of each point.
However, if further discussions lead to a consensus on these points, we would run an automatic upgrade procedure anyways.
@JackStouffer I would keep this open for another two or three days. I highly doubt that someone who feels strongly about e.g. synchronized( vs synchronized ( hasn't raised his/her voice yet, but let's better be safe than sorry.
std/concurrency.d
Outdated
| thisInfo.ident.mbox.get((T val) { | ||
| static if (T.length) | ||
| ret.field = val; | ||
| }, (LinkTerminated e) { throw e; }, (OwnerTerminated e) { throw e; }, (Variant val) { |
There was a problem hiding this comment.
I would use one for each statement, e.g.
(LinkTerminated e) { throw e; },
(OwnerTerminated e) { throw e; },
but it's already a huge improvement of the status quo!
std/concurrency.d
Outdated
| receiveTimeout( msecs(10), (int x) {}, (Variant x) {} ); | ||
| } ) ); | ||
| static assert(__traits(compiles, { | ||
| receiveTimeout(msecs(10), (int x) { }, (Variant x) { }); |
There was a problem hiding this comment.
Is there any reason why you convert {} to { }?
AFIAK {} is a very common style in Phobos (485 occurrences).
| bool register(string name, Tid tid) | ||
| { | ||
| synchronized( registryLock ) | ||
| synchronized (registryLock) |
There was a problem hiding this comment.
Q: Should we enforce a whitespace after synchronized within the entire Phobos codebase by adding this to the checks to the posix.mak?
| { | ||
| mutex_nothrow.unlock_nothrow(); | ||
| scope(exit) mutex_nothrow.lock_nothrow(); | ||
| scope (exit) mutex_nothrow.lock_nothrow(); |
There was a problem hiding this comment.
There are 271 occurrences of scope(exit) in Phobos, so maybe we should agree on a common style here as well?
There was a problem hiding this comment.
Same line if one liner. Apply other rules for block statements to scope when there are multiple lines.
|
(the Dscanner failure is due to the random behavior of |
JackStouffer
force-pushed
the
concurrency
branch
from
6120d6b to
af4bb44
Compare
4 participants
std/concurrency.d uses a non standard style, this brings it into line with the rest of the codebase
used the default dfmt options with some manual fixes.
CC @wilzbach