Is it possible to request a code review?

[IM]

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.
>
> A wild guess from me would be that you come from a C#/.NET 
> background OR you have worked heavily with the win api, am I 
> right?
>

No, I'm coming from a C++ background. Though I did a bit of C# 
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/SingleThreadTaskRunner.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/ThreadPoolTaskRunner.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/tasks/TaskSystem.d#L24. I got this from Ali's book.

Thank you!