Skip to content
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

#40 non-convertible types support #41

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maagy
Copy link

@maagy maagy commented Dec 15, 2021

Proposition of support of types which are not IConvertible. For example System.Numerics.Complex or some custom struct with overloaded operators.

@VitaliyMF
Copy link
Contributor

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).

@maagy
Copy link
Author

maagy commented Dec 15, 2021

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.

@VitaliyMF
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants