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!

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?

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.

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

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

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!

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.

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.

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;
}

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.

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.

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