Lambdas cleanup #1995
Lambdas cleanup #1995
Conversation
adamsitnik
commented
Dec 15, 2022
adamsitnik
commented
- don't create unnecessary lambdas (based on feedback from @KalleOlaviNiemitalo)
- Option ctor simplification (Argument ctor will throw for null anyway)
- avoid closure allocations
I guess you mean #1968 (comment). |
| @@ -210,10 +210,10 @@ void UnrecognizedArgumentError(ArgumentResult argumentResult) | |||
| /// <returns>The configured argument.</returns> | |||
| public Argument<T> AcceptLegalFilePathsOnly() | |||
| { | |||
| var invalidPathChars = Path.GetInvalidPathChars(); | |||
There was a problem hiding this comment.
Wasn't this meant to cache Path.GetInvalidPathChars() which returns a new char[] on each call?
There was a problem hiding this comment.
What we had was:
- calling
Path.GetInvalidPathChars()for everyAcceptLegalFilePathsOnlycall - storing its result in local variable (not a static field of the class, which would allow for caching the value)
- 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)
There was a problem hiding this comment.
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.
