Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use static lambda keyword #15154

Merged
merged 6 commits into from
Apr 12, 2021
Merged

Use static lambda keyword #15154

merged 6 commits into from
Apr 12, 2021

Conversation

iSazonov
Copy link
Collaborator

@iSazonov iSazonov commented Apr 4, 2021 •
edited
Loading

PR Summary

Use static lambda keyword where possible to make code readability better.

No functional changes.
No changes in generated IL code (one exclusion - one delegate was converted to static).

PR Context

PR Checklist

@iSazonov iSazonov added the CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log label Apr 4, 2021
@ghost ghost assigned adityapatwardhan Apr 4, 2021
@iSazonov iSazonov requested review from rjmholt and vexx32 April 4, 2021 15:43
vexx32
vexx32 approved these changes Apr 4, 2021
View reviewed changes
Copy link
Collaborator

@vexx32 vexx32 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not super familiar with this feature, but from what I've read of it and what I can see here, this looks like a good change to make. Nice work!

@iSazonov
Copy link
Collaborator Author

iSazonov commented Apr 5, 2021 •
edited
Loading

I'm not super familiar with this feature,

In general, Lambda does two allocations - (1) a delegate, (2) an instance of temporary class.
If a lambda uses only parameter arguments in its body Roslyn excludes second allocation (no closure created).
Roslyn already does the optimization for years. Now C# allows to add "static" keyword for such lambdas. This explicitly shows that the lambda does not use "external" variables - this makes a code readability better, also this protects from a regression (if the lambda is on hot path adding "external" variable can raise huge allocations.).

@TravisEz13
Copy link
Member

@PoshChan Please remind me in 1 day

@PoshChan
Copy link
Collaborator

PoshChan commented Apr 9, 2021

@TravisEz13, this is the reminder you requested 1 day ago

@TravisEz13
Copy link
Member

@PoshChan Please remind me in 30 minutes

@PoshChan
Copy link
Collaborator

@TravisEz13, this is the reminder you requested 30 minutes ago

@TravisEz13 TravisEz13 merged commit 91e7298 into PowerShell:master Apr 12, 2021
@TravisEz13 TravisEz13 added this to the 7.2.0-preview.5 milestone Apr 12, 2021
@iSazonov iSazonov deleted the use-static-lambda branch April 13, 2021 04:03
@ghost
Copy link

ghost commented Apr 14, 2021

🎉v7.2.0-preview.5 has been released which incorporates this pull request.:tada:

Handy links:

xtqqczze
xtqqczze reviewed Apr 15, 2021
View reviewed changes
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
@@ -5822,7 +5822,7 @@ private static TypeCompletionMapping[][] InitializeTypeCache()

#endregion Process_LoadedAssemblies

var grouping = entries.Values.GroupBy(t => t.Key.Count(c => c == '.')).OrderBy(g => g.Key).ToArray();
var grouping = entries.Values.GroupBy(static t => t.Key.Count(c => c == '.')).OrderBy(static g => g.Key).ToArray();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missed one :p

@xtqqczze
Copy link
Contributor

@iSazonov I wonder whether it would be worth proposing an analyzer for static lambda.

@iSazonov
Copy link
Collaborator Author

It is ask for Roslyn repo. (I asked in .Net Runtime without answer.)

@xtqqczze
Copy link
Contributor

It is ask for Roslyn repo. (I asked in .Net Runtime without answer.)

I can't find an existing issue.

@iSazonov
Copy link
Collaborator Author

iSazonov commented Apr 16, 2021 •
edited
Loading

It was my comment in an PR. :-)

You could review rules from last version of analyzers - perhaps such rule is there.

@xtqqczze
Copy link
Contributor

@xtqqczze I have submitted a new analyzer proposal: https://github.com/dotnet/roslyn-analyzers/issues/5027.

TravisEz13 added a commit that referenced this pull request Apr 16, 2021
@TravisEz13
Merge tag 'v7.2.0-preview.5'
32543c1 
[7.2.0-preview.5] - 2021-04-14

* Breaking Changes

- Make PowerShell Linux deb and RPM packages universal (#15109)
- Enforce AppLocker Deny configuration before Execution Policy Bypass configuration (#15035)
- Disallow mixed dash and slash in command line parameter prefix (#15142) (Thanks @davidBar-On!)

* Experimental Features

- `PSNativeCommandArgumentPassing`: Use `ArgumentList` for native executable invocation (breaking change) (#14692)

* Engine Updates and Fixes

- Add `IArgumentCompleterFactory` for parameterized `ArgumentCompleters` (#12605) (Thanks @powercode!)

* General Cmdlet Updates and Fixes

- Fix SSH remoting connection never finishing with misconfigured endpoint (#15175)
- Respect `TERM` and `NO_COLOR` environment variables for `$PSStyle` rendering (#14969)
- Use `ProgressView.Classic` when Virtual Terminal is not supported (#15048)
- Fix `Get-Counter` issue with `-Computer` parameter (#15166) (Thanks @krishnayalavarthi!)
- Fix redundant iteration while splitting lines (#14851) (Thanks @hez2010!)
- Enhance `Remove-Item -Recurse` to work with OneDrive (#14902) (Thanks @iSazonov!)
- Change minimum depth to 0 for `ConvertTo-Json` (#14830) (Thanks @kvprasoon!)
- Allow `Set-Clipboard` to accept empty string (#14579)
- Turn on and off `DECCKM` to modify keyboard mode for Unix native commands to work correctly (#14943)
- Fall back to `CopyAndDelete()` when `MoveTo()` fails due to an `IOException` (#15077)

* Code Cleanup

<details>

<summary>

<p>We thank the following contributors!</p>
<p>@xtqqczze, @iSazonov, @ZhiZe-ZG</p>

</summary>

<ul>
<li>Update .NET to <code>6.0.0-preview.3</code> (#15221)</li>
<li>Add space before comma to hosting test to fix error reported by <code>SA1001</code> (#15224)</li>
<li>Add <code>SecureStringHelper.FromPlainTextString</code> helper method for efficient secure string creation (#14124) (Thanks @xtqqczze!)</li>
<li>Use static lambda keyword (#15154) (Thanks @iSazonov!)</li>
<li>Remove unnecessary <code>Array</code> -&gt; <code>List</code> -&gt; <code>Array</code> conversion in <code>ProcessBaseCommand.AllProcesses</code> (#15052) (Thanks @xtqqczze!)</li>
<li>Standardize grammar comments in Parser.cs (#15114) (Thanks @ZhiZe-ZG!)</li>
<li>Enable <code>SA1001</code>: Commas should be spaced correctly (#14171) (Thanks @xtqqczze!)</li>
<li>Refactor <code>MultipleServiceCommandBase.AllServices</code> (#15053) (Thanks @xtqqczze!)</li>
</ul>

</details>

* Tools

- Use Unix line endings for shell scripts (#15180) (Thanks @xtqqczze!)

* Tests

- Add the missing tag in Host Utilities tests (#14983)
- Update `copy-props` version in `package.json` (#15124)

* Build and Packaging Improvements

<details>

<summary>

<p>We thank the following contributors!</p>
<p>@JustinGrote</p>

</summary>

<ul>
<li>Fix <code>yarn-lock</code> for <code>copy-props</code> (#15225)</li>
<li>Make package validation regex accept universal Linux packages (#15226)</li>
<li>Bump NJsonSchema from 10.4.0 to 10.4.1 (#15190)</li>
<li>Make MSI and EXE signing always copy to fix daily build (#15191)</li>
<li>Sign internals of EXE package so that it works correctly when signed (#15132)</li>
<li>Bump Microsoft.NET.Test.Sdk from 16.9.1 to 16.9.4 (#15141)</li>
<li>Update daily release tag format to  work with new Microsoft Update work (#15164)</li>
<li>Feature: Add Ubuntu 20.04 Support to install-powershell.sh (#15095) (Thanks @JustinGrote!)</li>
<li>Treat rebuild branches like release branches (#15099)</li>
<li>Update WiX to 3.11.2 (#15097)</li>
<li>Bump NJsonSchema from 10.3.11 to 10.4.0 (#15092)</li>
<li>Allow patching of preview releases (#15074)</li>
<li>Bump Newtonsoft.Json from 12.0.3 to 13.0.1 (#15084, #15085)</li>
<li>Update the <code>minSize</code> build package filter to be explicit (#15055)</li>
<li>Bump NJsonSchema from 10.3.10 to 10.3.11 (#14965)</li>
</ul>

</details>

* Documentation and Help Content

- Merge `7.2.0-preview.4` changes to master (#15056)
- Update `README` and `metadata.json` (#15046)
- Fix broken links for `dotnet` CLI (#14937)

[7.2.0-preview.5]: v7.2.0-preview.4...v7.2.0-preview.5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xtqqczze xtqqczze xtqqczze left review comments

@TravisEz13 TravisEz13 TravisEz13 approved these changes

@vexx32 vexx32 vexx32 approved these changes

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan adityapatwardhan is a code owner

@anmenaga anmenaga Awaiting requested review from anmenaga anmenaga is a code owner

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw daxian-dbw is a code owner

@SteveL-MSFT SteveL-MSFT Awaiting requested review from SteveL-MSFT SteveL-MSFT is a code owner

@rjmholt rjmholt Awaiting requested review from rjmholt

Assignees

@TravisEz13 TravisEz13

Labels
CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log
Projects
None yet
Milestone
7.2.0-preview.5
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@iSazonov @TravisEz13 @PoshChan @xtqqczze @vexx32 @adityapatwardhan