Add withLockVoid(_:) to NIOLock
#2276
Conversation
|
Can one of the admins verify this patch? |
10 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
@swift-server-bot test this please |
| @@ -140,6 +140,12 @@ extension NIOLock { | |||
| } | |||
| return try body() | |||
| } | |||
|
|
|||
| // specialise Void return (for performance) | |||
There was a problem hiding this comment.
Why is this more performant? This just calls down to self.withLock(_:).
There was a problem hiding this comment.
I wouldn't know, i just copy pasted the exact function that Lock had, including the comment above it 😅
There was a problem hiding this comment.
@Lukasa should we remove this doc comment?
There was a problem hiding this comment.
It's a regular comment, not a doc comment. You can remove it if you'd like.
There was a problem hiding this comment.
Just checking, is the comment still true about Lock? (perhaps because Lock is a class, unlike NIOLock?)
There was a problem hiding this comment.
The comment likely stopped being true when the method was marked @inlinable.
There was a problem hiding this comment.
Why don't we just suggest to use withLock? withLockVoid is pointless since about Swift 4.2 or so when @inlinable became available publicly.
There was a problem hiding this comment.
The honest answer is that I don’t think it’s worth the churn. While withLockVoid isn’t meaningfully better, plenty of existing code uses it and it makes the deprecation more painful than it needs to be.
There was a problem hiding this comment.
(Also it doesn’t totally seamlessly get replaced by withLock, due to single statement return value inference. That can cause you to need to explicitly discard the result, which this avoids.)
Motivation:
Lockhas recently been deprecated in favor ofNIOLock.The warnings claim that this is supposed to be a mere rename, but the API of
NIOLockmisseswithLockVoid(_:)compared toLock, which is unexpected to some users.Modifications:
NIOLocknow has thewithLockVoid(_:)too.Result:
LockandNIOLockwill have a more similar API and users can move toNIOLockeasier.This will also resolve #2275.