-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Make DependentHandle public #54246
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make DependentHandle public #54246
Changes from 1 commit
57fe91c
b1f54b5
b331b8f
a96fa3f
16fdf0e
748d88e
247fa5a
66d2ac5
4067ac3
1670339
96cfc91
6a8db56
1601d88
359938b
312851a
0145a76
4925877
1664a95
ca515b6
08df598
4e2b624
4e03297
25b34c2
01f32a3
b3963f2
34e1bcb
9fd1da4
d7146e0
c9c6325
c463d54
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,11 @@ namespace System.Runtime | |
| /// that object having a field or property (or some other strong reference) to a dependent object instance B. | ||
| /// </para> | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// The <see cref="DependentHandle"/> type is not thread-safe, and consumers are responsible for ensuring that | ||
| /// <see cref="Dispose"/> is not called concurrently with other APIs. Not doing so results in undefined behavior. | ||
| /// <para>The <see cref="Target"/> and <see cref="Dependent"/> properties are instead thread-safe.</para> | ||
| /// </remarks> | ||
| public struct DependentHandle : IDisposable | ||
|
||
| { | ||
| // ========================================================================================= | ||
|
|
@@ -68,6 +73,7 @@ public DependentHandle(object? target, object? dependent) | |
| /// Gets or sets the target object instance for the current handle. | ||
| /// </summary> | ||
| /// <exception cref="InvalidOperationException">Thrown if <see cref="IsAllocated"/> is <see langword="false"/>.</exception> | ||
| /// <remarks>This property is thread-safe.</remarks> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IsAllocated is missing a similar thread-safety remark. |
||
| public object? Target | ||
| { | ||
| get | ||
|
|
@@ -98,11 +104,13 @@ public object? Target | |
| /// Gets or sets the dependent object instance for the current handle. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// If it is necessary to retrieve both <see cref="Target"/> and <see cref="Dependent"/>, it is | ||
| /// recommended to use <see cref="GetTargetAndDependent"/> instead. This will result in better | ||
| /// performance and it will reduce the chance of unexpected behavior in some cases. | ||
| /// If it is needed to retrieve both <see cref="Target"/> and <see cref="Dependent"/>, it is necessary | ||
| /// to ensure that the returned instance from <see cref="Target"/> will be kept alive until <see cref="Dependent"/> | ||
| /// is retrieved as well, or it might be collected and result in unexpected behavior. This can be done by storing the | ||
| /// target in a local and calling <see cref="GC.KeepAlive(object)"/> on it after <see cref="Dependent"/> is accessed. | ||
| /// </remarks> | ||
| /// <exception cref="InvalidOperationException">Thrown if <see cref="IsAllocated"/> is <see langword="false"/>.</exception> | ||
| /// <remarks>This property is thread-safe.</remarks> | ||
| public object? Dependent | ||
| { | ||
| get | ||
|
|
@@ -129,26 +137,6 @@ public object? Dependent | |
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Retrieves the values of both <see cref="Target"/> and <see cref="Dependent"/>, if available. | ||
| /// </summary> | ||
| /// <returns>The values of <see cref="Target"/> and <see cref="Dependent"/>.</returns> | ||
| /// <exception cref="InvalidOperationException">Thrown if <see cref="IsAllocated"/> is <see langword="false"/>.</exception> | ||
| public (object? Target, object? Dependent) GetTargetAndDependent() | ||
| { | ||
| IntPtr handle = _handle; | ||
|
|
||
| if (handle == IntPtr.Zero) | ||
| { | ||
| ThrowHelper.ThrowInvalidOperationException(); | ||
| } | ||
|
|
||
| object? target = InternalGetTarget(handle); | ||
| object? secondary = InternalGetDependent(handle); | ||
|
|
||
| return (target, secondary); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets the target object instance for the current handle. | ||
| /// </summary> | ||
|
|
@@ -188,6 +176,7 @@ internal void UnsafeSetDependent(object? dependent) | |
| } | ||
|
|
||
| /// <inheritdoc cref="IDisposable.Dispose"/> | ||
| /// <remarks>This method is not thread-safe.</remarks> | ||
| public void Dispose() | ||
| { | ||
| // Forces dependentHandle back to non-allocated state (if not already there) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The details are nice, but this is way too long for a summary, which should be at most one sentence (it's what pops up in IntelliSense for a method). Can you move everything but the first sentence to remarks, editing it appropriately? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed this in c463d54, as well as all the other review comments below 🙂