Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Mark PowerShellAssemblyLoadContextInitializer with static modifier#13874

Merged
anmenaga merged 1 commit into
PowerShell:masterPowerShell/PowerShell:masterfrom
xtqqczze:static-PowerShellAssemblyLoadContextInitializerxtqqczze/PowerShell-PowerShell:static-PowerShellAssemblyLoadContextInitializerCopy head branch name to clipboard
Nov 4, 2020
Merged

Mark PowerShellAssemblyLoadContextInitializer with static modifier#13874
anmenaga merged 1 commit into
PowerShell:masterPowerShell/PowerShell:masterfrom
xtqqczze:static-PowerShellAssemblyLoadContextInitializerxtqqczze/PowerShell-PowerShell:static-PowerShellAssemblyLoadContextInitializerCopy head branch name to clipboard

Conversation

@xtqqczze

@xtqqczze xtqqczze commented Oct 25, 2020

Copy link
Copy Markdown
Contributor

PR Summary

Breaking change: this change removes the implicit public constructor for PowerShellAssemblyLoadContextInitializer.

Fix CA1822: Mark members as static.

Justification:

  • There appears to be no use case for creating an instance of PowerShellAssemblyLoadContextInitializer.
  • The class only contains one static method: SetPowerShellAssemblyLoadContext

PR Context

PR Checklist

@iSazonov

Copy link
Copy Markdown
Collaborator

Breaking change: this change removes the public constructor for PowerShellAssemblyLoadContextInitializer.

@xtqqczze I don't see the change.

@xtqqczze

xtqqczze commented Oct 28, 2020

Copy link
Copy Markdown
Contributor Author

Breaking change: this change removes the public constructor for PowerShellAssemblyLoadContextInitializer.

@xtqqczze I don't see the change.

The removed constructor is the implicit parameterless constructor.

See https://docs.microsoft.com/dotnet/csharp/programming-guide/classes-and-structs/constructors#parameterless-constructors

@iSazonov

iSazonov commented Oct 28, 2020

Copy link
Copy Markdown
Collaborator

What is a benefit of the change?

@xtqqczze

Copy link
Copy Markdown
Contributor Author

It is a fix for CA1822: Mark members as static.

Documentation categorizes this as a performance rule, but I would add it is also a possible correctness issue: we should not allow creation of an instance of PowerShellAssemblyLoadContextInitializer when there is no use case.

@iSazonov iSazonov added the CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log label Oct 28, 2020
@xtqqczze

xtqqczze commented Nov 4, 2020

Copy link
Copy Markdown
Contributor Author

on the other hand, this is breaking/changing how this function should be called by PS hosts, which will cause unnecessary additional work to change hosting code.

Correct me if I'm wrong, but the only this could be breaking is if someone attempts to create an instance of PowerShellAssemblyLoadContextInitializer. This seems unlikely as usage examples in documentation treat it as if it were already a static class:

"For such hosting scenarios, the native host needs to bootstrap by calling [PowerShellAssemblyLoadContextInitializer.SetPowerShellAssemblyLoadContext]"

https://github.com/PowerShell/PowerShell/blob/master/docs/host-powershell/README.md#special-hosting-scenario-for-native-host

@anmenaga

anmenaga commented Nov 4, 2020

Copy link
Copy Markdown

Oh, sorry. For some reason I thought that it was an instance method. :) Please ignore my previous post.

@anmenaga anmenaga merged commit 3444595 into PowerShell:master Nov 4, 2020
@xtqqczze xtqqczze deleted the static-PowerShellAssemblyLoadContextInitializer branch November 4, 2020 02:38
@iSazonov iSazonov added this to the 7.2.0-preview.1 milestone Nov 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.