www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Is it possible to request a code review?

reply IM <3di gm.com> writes:
I started learning D recently. As part of the learning process, I 
started working on some D projects to experiment and learn. My 
first project is a tasking system library that I plan to use in 
another D project I'm working on.

The goal is to be able to post and run any task asynchronously on 
current thread, new thread, or thread pool, and possibly getting 
a callback on the posting thread when the task has run to 
completion. I will probably expand it more as I go. I've just 
published it on DUB here: 
https://code.dlang.org/packages/libdtasks

Being new to D, I probably made many mistakes, or did things in a 
way where there's a more optimum one to do in D. I'm eager to 
know how to improve, and would like to know if there is any 
experienced D developer who is thankfully willing to take a look 
at it and give me feedback?

Thanks!
Dec 12 2017
next sibling parent reply bauss <jj_1337 live.dk> writes:
On Wednesday, 13 December 2017 at 03:15:11 UTC, IM wrote:

 Thanks!


First thing.

Generally in D module names are kept lower-case.

To give an example your:
AbstractTaskRunner.d
module tasks.AbstractTaskRunner;

Would usually be:
abstracttaskrunner.d
module tasks.abstracttaskrunner;

Of course it's not necessary, but it's how it usually is in 
idiomatic D.

The same goes for function names etc. they're usually kept 
camelCase and not PascalCase like you have it. Again it's not 
necessary, but that's idiomatic D.


background OR you have worked heavily with the win api, am I 
right?

Documentation can be done like this for multiline:

/**
* ...
* ...
* etc.
*/

Instead of:

/// ...
/// ...
/// etc.

This might just be a personal thing for me, but I always use /// 
for single-line documentation only.

Also you don't need to define constructors for classes, if they 
don't actually do something.

Ex. your SingleThreadTaskRunner class defines this:

~this()  safe {}

Integers are automatically initialized to int.init which is 0, so 
the following is can be rewritten from: (Find in a unittest)

int value = 0;

to:

int value;

Also empty lambdas like:

() { ... }

can be written like:

{ ... }

Also you have a few Hungarian notations, which is often not used 
in idiomatic D.

The key for an associative array __should__ always be treated 
const internally, so this:

ITaskRunner[const Tid]

shouldn't be necessary and doing:

ITaskRunner[Tid] should work just fine.

If you have experienced otherwise, then I'd argue it's a bug and 
you should report it.

Also statements like this:

immutable int currentNumber

are often preferred written like:

immutable(int) currentNumber

And getting too used to write:

immutable int currentNumber

might have you end up writing something like:

immutable int getAnImmutableNumber() { ... }

Which does not do what you would expect as it doesn't treat the 
return value as immutable, but instead "this" as immutable.

To make the return type immutable you'd write:

immutable(int) getAnImmutableNumber() { ... }

I don't know if you know that already, but just figured I'd give 
a heads up about that.

That was all I could see looking through your code real quick.

Take it with a grain of salt as some of the things might be 
biased.
Dec 12 2017
next sibling parent reply Jonathan M Davis <newsgroup.d jmdavisprog.com> writes:
On Wednesday, December 13, 2017 06:14:04 bauss via Digitalmars-d wrote:

 Documentation can be done like this for multiline:

 /**
 * ...
 * ...
 * etc.
 */

 Instead of:

 /// ...
 /// ...
 /// etc.


You can also do

/**
  ...
  ...
  etc.
*/

or

/++
  ...
  ...
  etc.
+/

Personally, I always use the last one if the ddoc comment is multiple lines,
and I've never understood why anyone would want all those extra *'s on the
side.

- Jonathan M Davis
Dec 12 2017
parent IM <3di gm.com> writes:
On Wednesday, 13 December 2017 at 07:30:55 UTC, Jonathan M Davis 
wrote:

 On Wednesday, December 13, 2017 06:14:04 bauss via 
 Digitalmars-d wrote:

 Documentation can be done like this for multiline:

 /**
 * ...
 * ...
 * etc.
 */

 Instead of:

 /// ...
 /// ...
 /// etc.


 You can also do

 /**
   ...
   ...
   etc.
 */

 or

 /++
   ...
   ...
   etc.
 +/

 Personally, I always use the last one if the ddoc comment is 
 multiple lines, and I've never understood why anyone would want 
 all those extra *'s on the side.

 - Jonathan M Davis


I think it is believed that the extra stars:

/**
  * <Insert docs here>
  * <Insert docs here>
  */

make the docs stand out and look better (as if they're inside a 
blockquote or something).

However, doc styles and code style in general is not my primary 
concern right now. I want dive deeply into D first, and worry 
about style later.
Dec 13 2017
prev sibling parent reply IM <3di gm.com> writes:
On Wednesday, 13 December 2017 at 06:14:04 UTC, bauss wrote:

 On Wednesday, 13 December 2017 at 03:15:11 UTC, IM wrote:

 Thanks!


 First thing.

 Generally in D module names are kept lower-case.

 To give an example your:
 AbstractTaskRunner.d
 module tasks.AbstractTaskRunner;

 Would usually be:
 abstracttaskrunner.d
 module tasks.abstracttaskrunner;

 Of course it's not necessary, but it's how it usually is in 
 idiomatic D.

 The same goes for function names etc. they're usually kept 
 camelCase and not PascalCase like you have it. Again it's not 
 necessary, but that's idiomatic D.


 background OR you have worked heavily with the win api, am I 
 right?



long time ago.


 Documentation can be done like this for multiline:

 /**
 * ...
 * ...
 * etc.
 */

 Instead of:

 /// ...
 /// ...
 /// etc.

 This might just be a personal thing for me, but I always use 
 /// for single-line documentation only.

 Also you don't need to define constructors for classes, if they 
 don't actually do something.

 Ex. your SingleThreadTaskRunner class defines this:

 ~this()  safe {}

 Integers are automatically initialized to int.init which is 0, 
 so the following is can be rewritten from: (Find in a unittest)

 int value = 0;

 to:

 int value;


I feel like it doesn't hurt to be explicit about it, but I get 
your point.


 Also empty lambdas like:

 () { ... }

 can be written like:

 { ... }


That's actually better, thanks!


 Also you have a few Hungarian notations, which is often not 
 used in idiomatic D.

 The key for an associative array __should__ always be treated 
 const internally, so this:

 ITaskRunner[const Tid]

 shouldn't be necessary and doing:

 ITaskRunner[Tid] should work just fine.

 If you have experienced otherwise, then I'd argue it's a bug 
 and you should report it.


I think I made it so, because I had a function with signature:

void Foo(const Tid tid) {
   // Use `tid` to access the associative array, which refused
   // to compile unless the key was marked `const` in the 
declaration.
}


 Also statements like this:

 immutable int currentNumber

 are often preferred written like:

 immutable(int) currentNumber

 And getting too used to write:

 immutable int currentNumber

 might have you end up writing something like:

 immutable int getAnImmutableNumber() { ... }

 Which does not do what you would expect as it doesn't treat the 
 return value as immutable, but instead "this" as immutable.

 To make the return type immutable you'd write:

 immutable(int) getAnImmutableNumber() { ... }

 I don't know if you know that already, but just figured I'd 
 give a heads up about that.


No, I didn't know. Thanks for mentioning!


 That was all I could see looking through your code real quick.

 Take it with a grain of salt as some of the things might be 
 biased.


Thanks. That was definitely helpful.

I was also looking for more feedback about the following:

   - Any candidate class that can be better switched to a struct 
or even a template?
   - The use of shared and __gshared, did I get those right? I 
find the TLS concept unpleasant, because it makes me unsure 
what's the right thing to do. For example:
     - If a class that instances of which will be accessed by 
multiple threads, should it be marked `shared`. For instance 
`TaskQueue` 
(https://gitlab.com/3d_immortal/libdtasks/blob/master/src/tasks/TaskQueue.d#L9).
     - What about the members of a class, do they ever need to be 
marked as shared?
     - Also, in this unittest : 
https://gitlab.com/3d_immortal/libdtasks/blob/master/src/tasks/SingleThre
dTaskRunner.d#L148, I didn't mark `number` as shared, even though it is
accessed by two threads, and I didn't see any unexpected behavior (because the
task runners implicitly synchronize access to it using tasks and replies). But
in the other unittest here:
https://gitlab.com/3d_immortal/libdtasks/blob/master/src/tasks/ThreadPo
lTaskRunner.d#L100, I marked `number` as shared just because I believed I have
to since it will be accessed by many threads arbitrarily.
     - Is this the idiomatic way to define a singleton in D?: 
https://gitlab.com/3d_immortal/libdtasks/blob/master/src/tas
s/TaskSystem.d#L24. I got this from Ali's book.

Thank you!
Dec 13 2017
next sibling parent reply rikki cattermole <rikki cattermole.co.nz> writes:
On 14/12/2017 3:57 AM, IM wrote:

snip


      - Is this the idiomatic way to define a singleton in D?: 
 https://gitlab.com/3d_immortal/libdtasks/blob/master/src/tas
s/TaskSystem.d#L24. 


You say singleton I think wrong.

Use free-functions and globals instead.

Singletons are always a code smell that OOP based languages like to say 
are a 'good thing'.

e.g.

module taskmgr;
import task;

private __gshared {
	Task[] tasks;
}

Task[] getTasks() {
	return tasks;
}

void clearTasks() {
	tasks = null;
}

void addTask(Task t) {
	tasks ~= t;
}
Dec 13 2017
parent IM <3di gm.com> writes:
On Thursday, 14 December 2017 at 04:12:33 UTC, rikki cattermole 
wrote:

 On 14/12/2017 3:57 AM, IM wrote:

 snip


      - Is this the idiomatic way to define a singleton in D?: 
 https://gitlab.com/3d_immortal/libdtasks/blob/master/src/tasks/TaskSystem.d#L24.


 You say singleton I think wrong.

 Use free-functions and globals instead.

 Singletons are always a code smell that OOP based languages 
 like to say are a 'good thing'.

 e.g.

 module taskmgr;
 import task;

 private __gshared {
 	Task[] tasks;
 }

 Task[] getTasks() {
 	return tasks;
 }

 void clearTasks() {
 	tasks = null;
 }

 void addTask(Task t) {
 	tasks ~= t;
 }


Sure, that works too, Thanks. However, we could leave the debate 
about the pros and cons of singletons for another day. That's not 
what my question was about though.
Dec 13 2017
prev sibling parent reply =?UTF-8?Q?Ali_=c3=87ehreli?= <acehreli yahoo.com> writes:
On 12/13/2017 07:57 PM, IM wrote:


      - Is this the idiomatic way to define a singleton in D?:
 


https://gitlab.com/3d_immortal/libdtasks/blob/master/src/tas
s/TaskSystem.d#L24. 


 I got this from Ali's book.


I think you got it from Adam D. Ruppe's book.

David Simcha's DConf 2013 presentation has Low-Lock Singleton Pattern at 
28 minute mark here:

   https://www.youtube.com/watch?v=yMNMV9JlkcQ&feature=youtu.be&t=1675

Ali
Dec 13 2017
parent IM <3di gm.com> writes:
On Thursday, 14 December 2017 at 07:47:58 UTC, Ali Çehreli wrote:

 On 12/13/2017 07:57 PM, IM wrote:


      - Is this the idiomatic way to define a singleton in D?:
 


 https://gitlab.com/3d_immortal/libdtasks/blob/master/src/tasks/TaskSystem.d#L24.


 I got this from Ali's book.


 I think you got it from Adam D. Ruppe's book.


Ooops, sorry, I thought I got it from your book because I've been 
reading in it recently. But no, I didn't get it from Adam D. 
Ruppe's book. Turned out that I actually got it from the 
initOnce() example in the docs here: 
https://dlang.org/library/std/concurrency/init_once.html


 David Simcha's DConf 2013 presentation has Low-Lock Singleton 
 Pattern at 28 minute mark here:

   
 https://www.youtube.com/watch?v=yMNMV9JlkcQ&feature=youtu.be&t=1675

 Ali


Thanks for the pointer. I'll check it out!
Dec 14 2017
prev sibling next sibling parent reply =?UTF-8?Q?Ali_=c3=87ehreli?= <acehreli yahoo.com> writes:
On 12/12/2017 07:15 PM, IM wrote:

 I started learning D recently.


Welcome! There is also the Learn newsgroup[1]. ;)

Ali

[1] Available through a forum interface here:

   http://forum.dlang.org/group/learn
Dec 12 2017
parent IM <3di gm.com> writes:
On Wednesday, 13 December 2017 at 07:02:47 UTC, Ali Çehreli wrote:

 On 12/12/2017 07:15 PM, IM wrote:

 I started learning D recently.


 Welcome! There is also the Learn newsgroup[1]. ;)

 Ali

 [1] Available through a forum interface here:

   http://forum.dlang.org/group/learn


Thanks, I posted a question there once already.
Dec 13 2017
prev sibling next sibling parent reply rjframe <dlang ryanjframe.com> writes:
On Wed, 13 Dec 2017 03:15:11 +0000, IM wrote:


 Being new to D, I probably made many mistakes, or did things in a way
 where there's a more optimum one to do in D. I'm eager to know how to
 improve, and would like to know if there is any experienced D developer
 who is thankfully willing to take a look at it and give me feedback?
 
 Thanks!


I like this API design.


I would separate each unit test to its own `unittest` block (you have 
three tests in a single block in `SingleThreadTaskRunner.d`); it doesn't 
really matter with the integrated runner, but if you later use a runner 
like unit-threaded[1] or tested[2] each unit test block becomes its own 
independent test, and they will all be run, even if one or more fails.


If `TaskQueue.Pop` returned the Task rather than used an out parameter, 
this (in SingleThreadTaskRunner.RunnerLoop):
```
while (true) {
      Task front;
      mQueue.Pop(front);
      if (!front)
        return;

      front();
    }
```
could become:
```
while (true) {
    auto front = mQueue.Pop();
    front ? front() : return;
}

```
Though that's probably debatable as to which is more readable. I would 
avoid `out` parameters on void functions; I would at least return a `bool` 
and test that instead (which is what `TryPop` does):
```
while (true) {
    Task front;
    if (!mQueue.Pop(front)) {
        return;
    }
    front();
```

I would probably switch the if block to the affirmative case, but that's 
personal preference.

`Pop` could call `TryPop` to eliminate code duplication. The same is true 
for other Try functions.


I'll ignore the style since others have discussed it and it's not what 

important; if I use your library in a project, it would be nice if API 
calls look like they belong in my code. That's why everybody recommends at 
least following the Phobos naming guidelines for published libraries.

I only skimmed other reviews, so this may have been mentioned; in 
`TaskQueue.d`, a private function is misspelled:
"AppednTaskThreadUnsafe" -> "AppendTaskThreadUnsafe".

--Ryan


[1]: http://code.dlang.org/packages/unit-threaded
[2]: http://code.dlang.org/packages/tested
Dec 14 2017
parent reply IM <3di gm.com> writes:
On Thursday, 14 December 2017 at 12:51:07 UTC, rjframe wrote:

 On Wed, 13 Dec 2017 03:15:11 +0000, IM wrote:


 [...]


 I like this API design.


 I would separate each unit test to its own `unittest` block 
 (you have three tests in a single block in 
 `SingleThreadTaskRunner.d`); it doesn't really matter with the 
 integrated runner, but if you later use a runner like 
 unit-threaded[1] or tested[2] each unit test block becomes its 
 own independent test, and they will all be run, even if one or 
 more fails.


Interesting. I haven't decided which tester runner to use yet, 
because the built in one is minimal and is getting me going. But 
thanks for the pointers. `tested` seems interesting because it's 
minimal and I like minimal things, however it seems it hasn't 
been touched for 2 years.


 If `TaskQueue.Pop` returned the Task rather than used an out 
 parameter,
 this (in SingleThreadTaskRunner.RunnerLoop):
 ```
 while (true) {
       Task front;
       mQueue.Pop(front);
       if (!front)
         return;

       front();
     }
 ```
 could become:
 ```
 while (true) {
     auto front = mQueue.Pop();
     front ? front() : return;
 }

 ```
 Though that's probably debatable as to which is more readable. 
 I would
 avoid `out` parameters on void functions; I would at least 
 return a `bool`
 and test that instead (which is what `TryPop` does):
 ```
 while (true) {
     Task front;
     if (!mQueue.Pop(front)) {
         return;
     }
     front();
 ```

 I would probably switch the if block to the affirmative case, 
 but that's personal preference.


I like that. I wouldn't switch it to affirmative because I like 
the early return and not having to add an `else`.


 `Pop` could call `TryPop` to eliminate code duplication. The 
 same is true for other Try functions.


 I'll ignore the style since others have discussed it and it's 

 actually is somewhat- important; if I use your library in a 
 project, it would be nice if API calls look like they belong in 
 my code. That's why everybody recommends at least following the 
 Phobos naming guidelines for published libraries.


That's a fair point, and I agree. I will consider switching to 
the Phobos
guidelines at some point.


 I only skimmed other reviews, so this may have been mentioned; 
 in `TaskQueue.d`, a private function is misspelled: 
 "AppednTaskThreadUnsafe" -> "AppendTaskThreadUnsafe".


Oops, thanks!


 --Ryan


 [1]: http://code.dlang.org/packages/unit-threaded
 [2]: http://code.dlang.org/packages/tested
Dec 15 2017
parent reply IM <3di gm.com> writes:
I'm still looking for feedback about these points:

   - Any candidate class that can be better switched to a struct 
or even a template?
   - The use of shared and __gshared, did I get those right? I 
find the TLS concept unpleasant, because it makes me unsure 
what's the right thing to do. For example:
     - If a class that instances of which will be accessed by 
multiple threads, should it be marked `shared`. For instance 
`TaskQueue` 
(https://gitlab.com/3d_immortal/libdtasks/blob/master/src/tasks/TaskQueue.d#L9).
     - What about the members of a class, do they ever need to be 
marked as shared?
     - Also, in this unittest : 
https://gitlab.com/3d_immortal/libdtasks/blob/master/src/tasks/SingleThre
dTaskRunner.d#L148, I didn't mark `number` as shared, even though it is
accessed by two threads, and I didn't see any unexpected behavior (because the
task runners implicitly synchronize access to it using tasks and replies). But
in the other unittest here:
https://gitlab.com/3d_immortal/libdtasks/blob/master/src/tasks/ThreadPo
lTaskRunner.d#L100, I marked `number` as shared just because I believed I have
to since it will be accessed by many threads arbitrarily.
Dec 15 2017
parent rikki cattermole <rikki cattermole.co.nz> writes:
On 15/12/2017 8:15 AM, IM wrote:

 I'm still looking for feedback about these points:
 
    - Any candidate class that can be better switched to a struct or even 
 a template?


Template(d/s) symbols are only useful for making things more generic for 
the purpose of code reuse.

Next it comes to two things regarding class/struct usage.

1) Do you really need the inheritance capabilities with vtables?
2) Is it being passed by value versus reference?

If you don't need inheritance but do need to pass by reference a final 
class may be appropriate.

If you don't need to copy it around at all, use neither (remember what I 
said about singletons?).

The purpose of all of this is to make it cheaper at runtime, 
performance. If you want to write rubbish code while getting the job 
done (which is not a wrong way of doing things), then ignore all of this 
and model it as you please.


    - The use of shared and __gshared, did I get those right? I find the 
 TLS concept unpleasant, because it makes me unsure what's the right 
 thing to do. For example:
      - If a class that instances of which will be accessed by multiple 
 threads, should it be marked `shared`. For instance `TaskQueue` 
 (https://gitlab.com/3d_immortal/libdtasks/blob/master/src/ta
ks/TaskQueue.d#L9). 
 
      - What about the members of a class, do they ever need to be marked 
 as shared?
      - Also, in this unittest : 
 https://gitlab.com/3d_immortal/libdtasks/blob/master/src/tasks/SingleThre
dTaskRunner.d#L148, 
 I didn't mark `number` as shared, even though it is accessed by two 
 threads, and I didn't see any unexpected behavior (because the task 
 runners implicitly synchronize access to it using tasks and replies). 
 But in the other unittest here: 
 https://gitlab.com/3d_immortal/libdtasks/blob/master/src/tasks/ThreadPo
lTaskRunner.d#L100, 
 I marked `number` as shared just because I believed I have to since it 
 will be accessed by many threads arbitrarily.


shared is little more than a way to tell the programmer what your 
intention is for a give bit of memory (with type safety but not codegen 
based safety).
Dec 15 2017
prev sibling parent reply Neia Neutuladh <neia ikeran.org> writes:
On Wednesday, 13 December 2017 at 03:15:11 UTC, IM wrote:

 https://code.dlang.org/packages/libdtasks


I'd get rid of the ITaskRunner interface and rename 
AbstractTaskRunner to TaskRunner. Interfaces are fine when you 
need them, but you rarely need them. You can just make 
PostTaskAndReply non-final and it's the same. It's slightly 
cheaper than calling the final method through an interface, even.

I'd also eliminate TaskSystem. You can use a thread-local 
TaskRunner pretty much the same way, and it's a bit weird to have 
policy based on thread ID. (Normally, you would have a 
threadpool-like thing for each type of task. Like one for serving 
web traffic, one for transcoding video, one for reading RSS 
feeds, etc.) Thread IDs can be reused, so if you forget to clean 
something up, you might end up reusing the wrong TaskRunner 
later. A thread-local variable also reduces the work you have to 
do to stop a thread.

In any case, the TaskRunner-per-thread thing is a policy decision 
that's not really appropriate for a library to make. You 
generally try to leave that to applications.

TaskQueue has a race condition, I think:


 Thread 1 calls Pop(). It acquires the mutex.
 Thread 1 observes there are no tasks available to pop. It waits 
 on the condition variable.
 Thread 2 calls Push(). It tries to acquire the mutex, but the 
 mutex is taken. It blocks forever.


Reading code is generally easier when that code follows the style 
guide for the language. https://dlang.org/dstyle.html and maybe 
run dfmt on your code. For instance, when you write `atomicOp 
!"+="`, it takes me about a full second to connect that to the 
common style of `atomicOp!"+="`. If I worked with your code for 
an hour, that would go away, but it's a bit of a speedbump for 
new contributors.

One other point of note about formatting: I never regret using 
braces around single-statement blocks. Like:

     while (!done && tasks.length == 0)
     {
         condition.wait;
     }

I just so frequently need to add logging or extra bookkeeping to 
statements like that that it's not even worth the time to try to 
omit braces.
Dec 15 2017
parent IM <3di gm.com> writes:
On Friday, 15 December 2017 at 21:34:48 UTC, Neia Neutuladh wrote:

 On Wednesday, 13 December 2017 at 03:15:11 UTC, IM wrote:

 https://code.dlang.org/packages/libdtasks


 I'd get rid of the ITaskRunner interface and rename 
 AbstractTaskRunner to TaskRunner. Interfaces are fine when you 
 need them, but you rarely need them. You can just make 
 PostTaskAndReply non-final and it's the same. It's slightly 
 cheaper than calling the final method through an interface, 
 even.


That's a fair point. Thanks.


 I'd also eliminate TaskSystem. You can use a thread-local 
 TaskRunner pretty much the same way, and it's a bit weird to 
 have policy based on thread ID. (Normally, you would have a 
 threadpool-like thing for each type of task. Like one for 
 serving web traffic, one for transcoding video, one for reading 
 RSS feeds, etc.) Thread IDs can be reused, so if you forget to 
 clean something up, you might end up reusing the wrong 
 TaskRunner later. A thread-local variable also reduces the work 
 you have to do to stop a thread.

 In any case, the TaskRunner-per-thread thing is a policy 
 decision that's not really appropriate for a library to make. 
 You generally try to leave that to applications.


Applications can instantiate multiple single thread task runners, 
each for a specific type of tasks as you mentioned earlier. They 
can also instantiate a thread pool task runners for tasks that 
are independent, don't need to run sequentially or any order, or 
on any particular thread.


 TaskQueue has a race condition, I think:


 Thread 1 calls Pop(). It acquires the mutex.
 Thread 1 observes there are no tasks available to pop. It 
 waits on the condition variable.
 Thread 2 calls Push(). It tries to acquire the mutex, but the 
 mutex is taken. It blocks forever.




That's not true. Waiting on a condition variable should unlocked 
the mutex first, the put the thread to sleep. That's how most 
implementations are. See for example: 
http://en.cppreference.com/w/cpp/thread/condition_variable/wait


 Reading code is generally easier when that code follows the 
 style guide for the language. https://dlang.org/dstyle.html and 
 maybe run dfmt on your code. For instance, when you write 
 `atomicOp !"+="`, it takes me about a full second to connect 
 that to the common style of `atomicOp!"+="`. If I worked with 
 your code for an hour, that would go away, but it's a bit of a 
 speedbump for new contributors.


Yeah, if you noticed, I use clang-format rather than dfmt. I 
found dfmt to be very buggy, especially when using 80-character 
line limit. It also produces formatted code that I don't like, 
especially when there's multiple indentations. clang-format 
doesn't support D unfortunately, so I have to add '// 
clang-format off .. on' here and there. It is clang-format that 
added that extra space after 'atomicOp!'. I wish it has full 
support for D like it does for Java and Javascript, but hey it 
mostly works, so fine.


 One other point of note about formatting: I never regret using 
 braces around single-statement blocks. Like:

     while (!done && tasks.length == 0)
     {
         condition.wait;
     }

 I just so frequently need to add logging or extra bookkeeping 
 to statements like that that it's not even worth the time to 
 try to omit braces.


I believe less braces and less indentation is better. I like to 
keep the code minimal with minimal whitespace around it, but 
that's just my personal pereferance.

Thank you very much for your review.
Dec 15 2017