fluent APIs should return least restrictive result #1964
Conversation
…an be used don't let types derived from `Option<T>` to override Argment property, require them to always pass it to the `ctor` so `_argument` and `Argument` always point to the same object inline Argument.None to reduce the number of methods compiled by the JIT at startup (it was used only in 2 places so it should be OK)
| _argument = argument; | ||
| } | ||
| | ||
| internal sealed override Argument Argument => _argument; |
There was a problem hiding this comment.
By making this property sealed we enforce all Option<T> derived types to always pass an instance of Argument<T> to the ctor. By doing that, we are always sure that _argument (the field) and Argument (the property) always refer to the same instance of an object. In the past it was not always a thing?
Another benefit is that when the derived types are being created, only one Argument<T> is being allocated.
jozkee left a comment
jozkee left a comment There was a problem hiding this comment.
Otherwise, LGTM. Sorry for breaking the SDK 😅.
| | ||
| public HelpOption(string[] aliases, Func<LocalizationResources> getLocalizationResources) | ||
| : base(aliases) | ||
| : base(aliases, null, new Argument<bool> { Arity = ArgumentArity.Zero }) |
There was a problem hiding this comment.
Could new Argument<bool> { Arity = ArgumentArity.Zero } be in a static and be re-used here and on VerisonOption?
There was a problem hiding this comment.
IIRC I've tried it in the past (when I was working on SCL perf) and it was impossible as some of the methods were allowing for mutation, but I am not 100% sure if this is the case now.
Once we are done with API-redesign and re-layering we are going to profile the startup scenario again and if it pops up, try to cache it. But initializing static fields also has a cost, so this will have to be carefully verified.
2 participants
fixes #1963