add core.sync.event to be used as Windows events by rainers · Pull Request #2517 · dlang/druntime

rainers · 2019-03-22T15:13:17Z

extracted from #2514

dlang-bot · 2019-03-22T15:13:19Z

Thanks for your pull request, @rainers!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + druntime#2517"

jacob-carlborg · 2019-03-22T18:41:58Z

src/core/sync/event.d

+
+version (Windows)
+{
+    import core.sys.windows.basetsd /+: HANDLE +/;


Why these comments?

This follows the example of https://github.com/dlang/druntime/pull/2400/files (selective imports create a public alias).

Hasn't this been fixed a year ago or so?

Oh well, looks like it's really still in deprecation, but there's a PR to remove them for good:

dlang/dmd#9393

That will go in very soon.

@rainers this has been merged now.

Not good enough:

test.d(3): Deprecation: core.sync.event.HANDLE is not visible from module test

jacob-carlborg · 2019-03-22T18:45:14Z

src/core/sync/event.d

+                pthread_cond_signal(&waiter.cond);
+                pthread_mutex_unlock(&waiter.mutex);
+            }
+            pthread_mutex_unlock(&m_mutex);


scope(exit)?

The function is nothrow anyway, so it doesn't make a difference in code generation. With a short function like this, having the symmetry show up in the function is slightly better IMO.

src/core/sync/event.d

jacob-carlborg · 2019-03-22T18:48:36Z

src/core/sync/event.d

+     *  manualReset  = the state of the event is not reset automatically after resuming waiting clients
+     *  initialState = initial state of the signal
+     */
+    void initialize(bool manualReset, bool initialState)


Why is this a separate method?

You cannot pass arguments to the constructor if it is part of another struct, e.g. https://github.com/dlang/druntime/pull/2514/files#diff-6615611ba51118215a271e3500c61122R2605. You have to use an explicit initialize-call then. __ctor could be an alternative, but I think it should not be part of the API.

Is there a downside to do evStart = Event(...) in Gcx.initialize?

I've explicitly added @disable this(this) as this could duplicate the handle reference.

Does not seem to prevent assignment: https://run.dlang.io/is/iefkBG.

Hmm, seems like opAssign should be disabled, too.

If that's required then I'm fine with using initialize instead of a constructor.

jacob-carlborg · 2019-03-23T11:28:44Z

src/core/sync/event.d

+        terminate();
+    }
+
+    void terminate()


No documentation.

jacob-carlborg · 2019-03-23T11:29:14Z

No changelog.

thewilsonator

Pending:

uncommented selective imports
changelog
initialize in place of constructor
terminate documentation

rainers · 2019-03-23T13:35:20Z

Added changelog and a bit more documentation. I've also added a bool to keep track of the initialization state of the posix version.

* uncommented selective imports

I think this should be done for all the imports once the dmd deprecation is gone (dlang/dmd#9393 doesn't help for this case yet).

thewilsonator · 2019-03-23T13:36:44Z

posix.mak:348: recipe for target 'checkwhitespace' failed

changelog/core_sync_event.dd

kinke · 2019-03-23T22:32:07Z

Rainer, have you explored whether pthread_cond_broadcast and a single condition variable could work too?

rainers · 2019-03-24T09:44:28Z

Rainer, have you explored whether pthread_cond_broadcast and a single condition variable could work too?

Didn't know about it, now replaced the manual implementation.

wilzbach · 2019-03-24T10:46:45Z

src/core/sync/event.d

+    }
+}
+
+// Test single-thread (non-shared) use.


Maybe make this example public, s.t. there's at least one user-facing example.

I think this example is a pretty bad one as it doesn't involve multiple threads. Maybe better to move the example from the changelog into the documentation?

Sure. I just saw that the module is currently without any public example.

Geod24 · 2019-03-25T03:47:48Z

src/core/sync/event.d

+        event.terminate();
+    }
+}
+---


Why isn't it a documented unittest instead ?

I think its already a bit too long for a really good example, and making it actually runnable would make it even longer.

rainers requested review from MartinNowak and andralex as code owners March 22, 2019 15:13

rainers requested review from PetarKirov, WalterBright and wilzbach as code owners March 22, 2019 15:13

rainers force-pushed the core_sync_event branch 7 times, most recently from 3f7ef75 to 0232a3f Compare March 22, 2019 16:38

jacob-carlborg reviewed Mar 22, 2019

View reviewed changes

src/core/sync/event.d Outdated Show resolved Hide resolved

jacob-carlborg reviewed Mar 22, 2019

View reviewed changes

src/core/sync/event.d Outdated Show resolved Hide resolved

jacob-carlborg reviewed Mar 22, 2019

View reviewed changes

src/core/sync/event.d Outdated Show resolved Hide resolved

jacob-carlborg reviewed Mar 22, 2019

View reviewed changes

rainers force-pushed the core_sync_event branch 2 times, most recently from e0006b2 to 0fb8576 Compare March 23, 2019 10:22

jacob-carlborg reviewed Mar 23, 2019

View reviewed changes

src/core/sync/event.d

terminate();

}

void terminate()

Copy link
Copy Markdown

Contributor

jacob-carlborg Mar 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No documentation.

thewilsonator added the Pending Changelog label Mar 23, 2019

thewilsonator approved these changes Mar 23, 2019

View reviewed changes

rainers force-pushed the core_sync_event branch from 0fb8576 to 018e340 Compare March 23, 2019 13:30

rainers force-pushed the core_sync_event branch from 018e340 to 7bc28a8 Compare March 23, 2019 13:36

jacob-carlborg reviewed Mar 23, 2019

View reviewed changes

changelog/core_sync_event.dd Outdated Show resolved Hide resolved

rainers force-pushed the core_sync_event branch 2 times, most recently from 7171e27 to 05e84ca Compare March 23, 2019 14:27

kinke reviewed Mar 23, 2019

View reviewed changes

changelog/core_sync_event.dd Outdated Show resolved Hide resolved

add core.sync.event to be used as Windows events

46987a8

rainers force-pushed the core_sync_event branch from 05e84ca to 46987a8 Compare March 23, 2019 14:52

rainers force-pushed the core_sync_event branch from 0099e2a to a5d9df2 Compare March 24, 2019 09:45

rainers removed the Pending Changelog label Mar 24, 2019

wilzbach reviewed Mar 24, 2019

View reviewed changes

use pthread_cond_broadcast instead of manual implementation

ba5bbeb

rainers force-pushed the core_sync_event branch from a5d9df2 to ba5bbeb Compare March 24, 2019 11:02

move exampple from changelog to doc

89f4930

thewilsonator added the auto-merge-squash label Mar 25, 2019

dlang-bot merged commit 334b9df into dlang:master Mar 25, 2019

Geod24 reviewed Mar 25, 2019

View reviewed changes

Uh oh!

Conversation

rainers commented Mar 22, 2019

Uh oh!

dlang-bot commented Mar 22, 2019

Bugzilla references

Testing this PR locally

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacob-carlborg commented Mar 23, 2019

Uh oh!

thewilsonator left a comment

Choose a reason for hiding this comment

Uh oh!

rainers commented Mar 23, 2019

Uh oh!

thewilsonator commented Mar 23, 2019

Uh oh!

Uh oh!

Uh oh!

kinke commented Mar 23, 2019

Uh oh!

rainers commented Mar 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants