-
Notifications
You must be signed in to change notification settings - Fork 50
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.
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
Adds HasValue #142
Adds HasValue #142
Conversation
@ndrwrbgs added experimental support as a new member in PR. Haven't tested performance, allocations etc. Naming. Not sure. Wrestled a bit between |
[ContractAnnotation("value:null => halt")] | ||
public T HasValue<T>([NoEnumeration, ValidatedNotNull] T value, [InvokerParameterName] string paramName = null, OptsFn optsFn = null) => value switch | ||
{ | ||
ValueType _ => value, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like ValueType
!= struct
- this syntax is new to me so I can only cursorily look over it, since I would've expected int? (I think Nullable is a struct?) to be ValueType, but your tests show that must not be true
After measuring the boxing path was more performant (both time and alloc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've heard rumors JIT might be able to optimize away a ReferenceEquals(null) (so maybe an == too? Though that can be overridden whereas Reference can't) -- I'm curious if the call can be inlined and removed for a generic that happens to be a ValueType, but it's not really important enough to suggest any changes. Either it is and users rejoice, or it's not and there's nothing more to be done than what you already supplied as warning in the xmldoc comment ;)
Resolves #140 by adding
HasValue
that supports value types and reference types.