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
Improve operator overloading logic #1011
Conversation
* cleanup and refactor code * use struct record instead of value tuples and custom code * prefer more specific types over object * prefer CLR array against JS array match over changing types
|
||
var castOperator = _knownCastOperators.GetOrAdd(key, _ => | ||
valueType.GetOperatorOverloadMethods() | ||
.Concat(type.GetOperatorOverloadMethods()) | ||
.FirstOrDefault(m => type.IsAssignableFrom(m.ReturnType) | ||
&& (m.Name == "op_Implicit" || m.Name == "op_Explicit"))); | ||
.FirstOrDefault(m => type.IsAssignableFrom(m.ReturnType) && m.Name is "op_Implicit" or "op_Explicit")); |
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.
🤕
#else | ||
private static readonly ConcurrentDictionary<string, MethodDescriptor> _knownOperators = new ConcurrentDictionary<string, MethodDescriptor>(); | ||
#endif | ||
private readonly record struct OperatorKey(string OperatorName, Type Left, Type Right); |
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.
if (method.Parameters.Length == 0 && arguments.Length == 0) | ||
{ | ||
// perfect | ||
return 0; | ||
} |
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.
Does this mean method will match perfectly if argument lengths are the same, regardless of the argument types? Isn't this wrong, or am I missing something?
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.
Method has no parameters and JS call has no arguments? What is there to match?
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 had a little brain fart. I saw another thing when reviewing.
Sorry for the trouble. Thank you.
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.
No worries, I always appreciate reviews and and extra pair of eyes 👍🏻
Hopefully the overload selection logic now a bit easier to read and maintain.
struct record
instead of value tuples and custom codeTryOperatorOverloading
isTry
pattern, it should not throw on conversion error, just return falsefixes #1004
fixes #1008