- 
                Notifications
    You must be signed in to change notification settings 
- Fork 5.2k
Add generic And<T> and Or<T> methods to System.Threading.Interlocked #120978
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
Conversation
Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
…methods Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
| @copilot find existing usages of  | 
Found and replaced all usages of `Interlocked.And(ref Unsafe.As<EnumType, IntType>(ref field), value)` and `Interlocked.Or(ref Unsafe.As<EnumType, IntType>(ref field), value)` with the new generic API `Interlocked.And(ref field, value)` and `Interlocked.Or(ref field, value)`. Affected files: - System.Private.CoreLib/ComWrappers.cs (CreateComInterfaceFlagsEx enum) - System.Net.Http/CacheControlHeaderValue.cs (Flags enum) - System.Private.Uri/Uri.cs (Flags enum) - System.Private.Uri/UriSyntax.cs (Uri.Flags enum) - System.Private.Uri/UriScheme.cs (Uri.Flags enum) Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
| 
 Done! Found and replaced 7 usages across 5 files: System.Private.CoreLib 
 System.Net.Http 
 System.Private.Uri 
 All calls now use  | 
| @copilot in importercallls.cpp for:             case NI_System_Threading_Interlocked_Or:
            case NI_System_Threading_Interlocked_And:can you disable the intrinsic expansion for  | 
Modified importercalls.cpp to only expand the intrinsic for And/Or operations when genActualType(callType) is TYP_INT or TYP_LONG. Other types (like byte, short) now fall back to the managed implementation which uses CompareExchange loops. This ensures that: - int/uint and long/ulong use native atomic instructions - byte/sbyte and short/ushort use managed CompareExchange-based implementation - Generic method calls work correctly for all supported types Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
| 
 ... Done! Modified  The change: 
 All 611 System.Threading tests pass. (256f950) | 
        
          
                src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | @copilot Change the added tests: 
 | 
- Use callType directly in JIT intrinsic check as suggested by @EgorBo - Allow float/double support (they work via bitcast to int/long for bitwise operations) - Convert all And/Or generic tests to [Theory] with [InlineData] - Add 5-10 test cases per type including negative values and edge cases - Add comprehensive tests for float/double types - Total test count increased from 611 to 712 All tests pass successfully. Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
| 
 Done! All test changes implemented: 
 Test count increased from 611 to 712. All tests pass. (d549cdd) | 
| @jkotas @stephentoub does this look good? I've used TypeCode to simplify enum check, without it was: if ((!typeof(T).IsPrimitive && !typeof(T).IsEnum) ||
    typeof(T) == typeof(float) ||
    typeof(T) == typeof(double) ||
    (typeof(T).IsEnum && (typeof(T).GetEnumUnderlyingType() == typeof(float) ||
                            typeof(T).GetEnumUnderlyingType() == typeof(double))))
{
    throw new NotSupportedException();
} | 
| 
 What does that do to inlining budgets, etc.? | 
| 
 Ah, that is a good question, it looks like Type.GetTypeCode(typeof(T)) is not a direct intrinsic and relies on it being inlined, so probably it's not a good choice indeed, didn't know that 🤔 | 
| Right. Also,  | 
        
          
                src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs
          
            Show resolved
            Hide resolved
        
              
          
                src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs
          
            Show resolved
            Hide resolved
        
      | 
 I've reverted to GetEnumUnderlyingType. Does it look good otherwise? | 
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.
LGTM!
        
          
                src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | @copilot address the feedback over the NotSupportedException check to make it single level. | 
Changed the NotSupportedException validation logic from nested if statements to a single-level if with a compound condition using logical operators. This addresses PR review feedback to: - Remove unnecessary nesting - Fix indentation issues - Make the logic more readable and maintainable The condition now checks all invalid type scenarios in one expression: - Non-primitive and non-enum types - float and double primitives - Enums backed by float or double All 705 tests pass successfully. Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
| /ba-g deadletter | 
| /ba-g deadletter | 
Summary
This PR adds generic versions of the
AndandOrmethods toSystem.Threading.Interlockedas specified in API proposal #114568.Changes
And<T>(ref T, T) where T : structmethodOr<T>(ref T, T) where T : structmethodInterlocked.And/Or(ref Unsafe.As...)with new generic APIImplementation Details
byte,sbyte,short,ushort): Uses CompareExchange-based loop (managed fallback)int,uint): Delegates to int And/Or with bitcast (JIT intrinsic)long,ulong): Delegates to long And/Or with bitcast (JIT intrinsic)Type.GetEnumUnderlyingType()check (JIT-foldable)CompareExchange<T>JIT Changes
Modified
importercalls.cppto restrict intrinsic expansion:TYP_INTandTYP_LONGare expanded as atomic operationscallTypeconsistently for type checksType Rejection
The methods reject:
Type.GetEnumUnderlyingType())The validation check is a single-level if statement with proper indentation:
Documentation
The XML documentation clearly states the restrictions:
Test Coverage
Converted all tests to
[Theory]with[InlineData]covering:Usages Replaced
Replaced 7 call sites across the codebase:
System.Private.CoreLib/ComWrappers.cs(2 calls)System.Net.Http/CacheControlHeaderValue.cs(2 calls)System.Private.Uri/Uri.cs(1 call)System.Private.Uri/UriSyntax.cs(1 call)System.Private.Uri/UriScheme.cs(1 call)All usages now use the cleaner generic API without
Unsafe.Ascasts.Test Results
✅ All 705 System.Threading tests pass (0 errors, 0 failures)
Security
✅ CodeQL analysis passed with no issues
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.