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!
