[NO MERGE] BitOps analysis CoreFX (WIP)#34917
Conversation
| internal static void ClearBit(ref int value, uint bitmask) | ||
| { | ||
| value = (int)(((uint)value) & ((uint)(~bitmask))); | ||
| BitOps.ClearBit(ref value, (int)bitmask); |
There was a problem hiding this comment.
Motivation to use uint for declared offset param
There was a problem hiding this comment.
I need to do some more analysis of the original code to make sure that the refactor is functionally equivalent.
| internal static void SetBit(ref int value, uint bitmask) | ||
| { | ||
| value = (int)(((uint)value) | ((uint)bitmask)); | ||
| BitOps.InsertBit(ref value, (uint)bitmask); |
There was a problem hiding this comment.
I need to do some more analysis of the original code to make sure that the refactor is functionally equivalent.
|
Thanks. As the issue hasn't been reviewed/approved yet, I'm going to close this. Obviously you can continue to iterate on it and use it as reference until it's actionable as a PR. |
|
@stephentoub, at this point the So, we should be able to take this regardless of the status of the corresponding API proposal (given that it is all internal). I imagine we will want to take this as well, given that it simplifies our existing code (by ensuring we have a single helper method, rather than 5, 10, or more duplicates). |
Internal where? It's not in the PR. That's also not how the PR is described. And it wouldn't need to be a no-merge / wip if it were just about changing some call sites to use some internal helper... if/when that's what it is, it can be opened for that. |
|
Its internal in CoreCLR now (with one operation) and there is a corresponding change against CoreCLR doing what I described above (centralizing the rest of the bitops already used in CoreCLR). I agree this PR could wait until the CoreCLR side is merged, however. |
4 participants
Update call sites for dotnet/coreclr#22225
Note that this PR will not build right now - it is being used to analyze the consolidation of call sites for https://github.com/dotnet/corefx/issues/32269.
cc @tannergooding