Is it possible to request a code review?

[IM]

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 
> not what you're looking for, but this does look like C#. Style 
> 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