[SourceKit] Add Semaphore to SourceKitSupport's concurrency module #3085
Conversation
|
@swift-ci please test |
| return Impl::wait(ImplObj, milliseconds); | ||
| } | ||
|
|
||
| Semaphore(const Semaphore &Other) { |
There was a problem hiding this comment.
Having semaphores be copy-constructible sounds like a bad idea to me. We should be passing them by reference. What do you think?
There was a problem hiding this comment.
Although I can be convinced otherwise.
There was a problem hiding this comment.
Sure, I do think that it makes sense to treat a semaphore as an object intended to be shared rather than copied. I think I was following the example of WorkQueue in this case.
I haven't written much C++ recently and am still trying to get a better understanding of which types of constructors are appropriate. What do you think about the move constructor here? Does it make sense as-is?
There was a problem hiding this comment.
The move constructor LGTM. The copy constructor you can just define as = delete, and that should make Semaphore a move-only type.
operator=(Semaphore &&Other) is actually OK for a move-only type.
briancroom
force-pushed
the
sourcekit-semaphore
branch
from
13af85d to
d578925
Compare
|
Updated according to @gribozavr's suggestion. |
|
@swift-ci please test |
| } | ||
|
|
||
| bool Semaphore::Impl::wait(Ty Obj, long milliseconds) { | ||
| dispatch_time_t timeout = dispatch_time(DISPATCH_TIME_NOW, milliseconds * NSEC_PER_MSEC); |
|
LGTM modulo comment, but it looks strange that |
|
LGTM if Dmitri's comment is addressed. |
Includes a libdispatch-based implementation.
briancroom
force-pushed
the
sourcekit-semaphore
branch
from
d578925 to
4795841
Compare
|
Excellent! Thanks for the review, guys. |
What's in this pull request?
This adds a basic semaphore class to the existing concurrency module in
SourceKitSupport, together with an implementation that useslibdispatch. It also migrates several bits of code to use the new class instead ofdispatch_semaphore_tdirectly.The most immediate motivation for this change was to remove all explicit
libdispatchusage from SourceKit's unit tests which was a barrier to getting them to build on Linux.This is related to #2793
Related bug number: (SR-1676)
Before merging this pull request to apple/swift repository:
Triggering Swift CI
The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:
Smoke Testing
Validation Testing
Lint Testing
Note: Only members of the Apple organization can trigger swift-ci.
@gribozavr @modocache @benlangmuir