Open maagy opened 2 years ago
Changes in math operations seems safe, however in comparisons the behavior may be not fully backward compatible, in particular:
Current impl:
return c1.Cmp.Compare(c1.Value, c2.Value)==0;
Proposed:
return c1.Cmp.Compare(c1.Value, c2.Value)==0;
if(c1.Value == null || c2.Value == null || c1.Value is IComparable or IList || c2.Value is IComparable or IList)
return c1.Cmp.Compare(c1.Value, c2.Value)==0;
return (dynamic) c1?.Value == (dynamic) c2?.Value;
Comparer implementation may be custom, and this may (potentially) change existing expressions evaluation.
I think it would be better to leave comparisons as-is, and use (dynamic) c1?.Value == (dynamic) c2?.Value
inside ValueComparer class as a fallback when values cannot be compared in a standard way (currently this situation leads to "Cannot compare" exception).
There would be problems with operators like >, <,... because operator == usually returns bool, which is insufficient for decision about greater value.
It would be possible to modify operations this way (when comparer is returning null if there is no known way to compare):
public static bool operator >(LambdaParameterWrapper c1, LambdaParameterWrapper c2) {
int? comparison = c1.Cmp.Compare(c1.Value, c2.Value);
if(comparison != null)
return comparison>0;
return (dynamic) c1.Value > (dynamic) c2.Value;
}
But there is still operators ==, !=. They don't know which way the comparer handles null values. I could pull NullComparison property up to IValueComparer but that would be a breaking change too. Or I could try to cast current comparer to NReco.Linq.ValueComparer and handle it according to selected mode. But what would be the result of comparison when the cast failes?
Maybe there is another way. I just can't see it now. I would understand if you refuse this PR.
There would be problems with operators like >, <,... because operator == usually returns bool, which is insufficient for decision about greater value.
I don't see any problems here, as currently all comparisons inside LambdaParameterWrapper are based on IValueComparer.Compare
int result which works in the same way as IComparer<>
. Nulls handling should remain the same as it is currently implemented in the default comparer implementation.
Maybe there is another way. I just can't see it now. I would understand if you refuse this PR.
I cannot merge current PR's changeset as it may break backward compatibility - this is unacceptable for the expressions parser because often expressions are entered by the end-user (and this incompatibility may appear as a wrong evaluation result).
I'll try to adopt your approach (when I'll have a bit of free time for that) to make possible to use System.Numerics.Complex (or another custom types) and guarantee backward compatibility.
Proposition of support of types which are not IConvertible. For example System.Numerics.Complex or some custom struct with overloaded operators.