Skip to content

Lambdas cleanup #1995

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

Merged
merged 3 commits into from
Dec 15, 2022
Merged

Lambdas cleanup #1995

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5a3db98
don't create unnecessary lambdas
adamsitnik Dec 15, 2022
20525f6
the Argument<T> ctor will throw for null anyway
adamsitnik Dec 15, 2022
0f80fd2
avoid closure allocations
adamsitnik Dec 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions src/System.CommandLine/Argument{T}.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public Argument(
Func<T> defaultValueFactory,
string? description = null) : this(name, description)
{
SetDefaultValueFactory(() => defaultValueFactory());
SetDefaultValueFactory(defaultValueFactory);
}

/// <summary>
Expand All @@ -63,7 +63,7 @@ public Argument(
/// <exception cref="ArgumentNullException">Thrown when <paramref name="defaultValueFactory"/> is null.</exception>
public Argument(Func<T> defaultValueFactory) : this()
{
SetDefaultValueFactory(() => defaultValueFactory());
SetDefaultValueFactory(defaultValueFactory);
}

/// <summary>
Expand Down Expand Up @@ -132,7 +132,7 @@ public Argument(Func<ArgumentResult, T> parse, bool isDefault = false) : this(nu
/// <param name="value">The default value for the argument.</param>
public void SetDefaultValue(T value)
{
SetDefaultValueFactory(() => value);
SetDefaultValueFactory(_ => value);
}

/// <summary>
Expand Down Expand Up @@ -210,10 +210,10 @@ void UnrecognizedArgumentError(ArgumentResult argumentResult)
/// <returns>The configured argument.</returns>
public Argument<T> AcceptLegalFilePathsOnly()
{
var invalidPathChars = Path.GetInvalidPathChars();
Copy link
Member

Choose a reason for hiding this comment

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

Wasn't this meant to cache Path.GetInvalidPathChars() which returns a new char[] on each call?

Copy link
Member Author

@adamsitnik adamsitnik Dec 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

What we had was:

  1. calling Path.GetInvalidPathChars() for every AcceptLegalFilePathsOnly call
  2. storing its result in local variable (not a static field of the class, which would allow for caching the value)
  3. allocating a closure (a lambda with a state)

What I am offering here is lack of closure allocation. I would prefer to avoid caching the result of Path.GetInvalidPathChars() in a static field as long as profiling don't prove that it's needed (adding static fields can hurt startup scenarios, it's not always a free lunch)

Copy link

@KalleOlaviNiemitalo KalleOlaviNiemitalo Dec 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Each Argument<T>.AcceptLegalFilePathsOnly call created a separate cache, so it was only effective if the same Argument<T> was validated multiple times, which I think would only happen if the application reads and parses multiple commands. If the application has subcommands, the eager Path.GetInvalidPathChars() call could be wasteful because it happens even if the subcommand is not invoked.

If caching is desired here, I think a static field in a non-generic class could work.


Validators.Add(result =>
Validators.Add(static result =>
{
var invalidPathChars = Path.GetInvalidPathChars();

for (var i = 0; i < result.Tokens.Count; i++)
{
var token = result.Tokens[i];
Expand All @@ -239,10 +239,10 @@ public Argument<T> AcceptLegalFilePathsOnly()
/// <returns>The configured argument.</returns>
public Argument<T> AcceptLegalFileNamesOnly()
{
var invalidFileNameChars = Path.GetInvalidFileNameChars();

Validators.Add(result =>
Validators.Add(static result =>
{
var invalidFileNameChars = Path.GetInvalidFileNameChars();

for (var i = 0; i < result.Tokens.Count; i++)
{
var token = result.Tokens[i];
Expand Down
4 changes: 2 additions & 2 deletions src/System.CommandLine/Option{T}.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,15 +51,15 @@ public Option(
Func<T> defaultValueFactory,
string? description = null)
: this(name, description,
new Argument<T>(defaultValueFactory ?? throw new ArgumentNullException(nameof(defaultValueFactory))))
new Argument<T>(defaultValueFactory))
{ }

/// <inheritdoc/>
public Option(
string[] aliases,
Func<T> defaultValueFactory,
string? description = null)
: this(aliases, description, new Argument<T>(defaultValueFactory ?? throw new ArgumentNullException(nameof(defaultValueFactory))))
: this(aliases, description, new Argument<T>(defaultValueFactory))
{
}

Expand Down