Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Update BitOperations callsites - #35775

Closed
grant-d wants to merge 4 commits into
dotnet:masterfrom
grant-d:grant-d.bitops-callsites
Closed

Update BitOperations callsites#35775
grant-d wants to merge 4 commits into
dotnet:masterfrom
grant-d:grant-d.bitops-callsites

Conversation

@grant-d

@grant-d grant-d commented Mar 5, 2019

Copy link
Copy Markdown

Update known callsites in CoreFX to use BitOperations
See #35606
And #34917

cc @tannergooding, @jkotas

Comment thread src/Common/src/System/Net/WebSockets/ManagedWebSocket.cs
@grant-d

grant-d commented Mar 5, 2019

Copy link
Copy Markdown
Author

Looks like I may need to fix code depending on target.
Should I keep the legacy code as a software fallback for netstandard ?

build.cmd -restore -build -configuration Release -ci -buildtests -arch x86 -framework netfx …

PEHeaderBuilder.cs(68,69): error CS0103: The name 'BitOperations' does not exist in the current context [D:\a\1\s\src\System.Reflection.Metadata\src\System.Reflection.Metadata.csproj]

@tannergooding

Copy link
Copy Markdown
Member

Should I keep the legacy code as a software fallback for netstandard

Yes, this will likely be required until BitOperations makes it into netstandard. CC @terrajobst

@jkotas

jkotas commented Mar 18, 2019

Copy link
Copy Markdown
Member

Adding BitOperations to netstandard is not going to alleviate this.

The real problem is that System.Reflection.Metadata is targeting existing netstandard versions that is not changing anytime soon.

@grant-d

grant-d commented Mar 18, 2019

Copy link
Copy Markdown
Author

Should I close this PR then - I am not sure it's worth it?

@jkotas

jkotas commented Mar 18, 2019

Copy link
Copy Markdown
Member

I think the change in ManagedWebSocket.cs is good. The changes in System.Reflection.Metadata are not worth it.

@grant-d grant-d changed the title [WIP] Update BitOperations callsites Update BitOperations callsites Mar 18, 2019
@grant-d

grant-d commented Mar 18, 2019

Copy link
Copy Markdown
Author

OK, manually reverted changes to System.Reflection.
The only real change is in ManagedWebSocket.cs, with some VS auto-format fixes in the reverted files.

@grant-d

grant-d commented Mar 18, 2019

Copy link
Copy Markdown
Author

All the NetFx targets are going to have the same problem. I don't think multi-targeting is worth it in this case since after inlining it has no functional benefit. It's really just about pretty code.

@grant-d grant-d closed this Mar 18, 2019
@grant-d
grant-d deleted the grant-d.bitops-callsites branch March 18, 2019 18:36
@jkotas

jkotas commented Mar 18, 2019

Copy link
Copy Markdown
Member

Ok, I have not realized that ManagedWebSocket.cs is included in netstandard project as well.

@karelz karelz added this to the 3.0 milestone Mar 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants