Translate generic math calls using the existing Math translators #31322
Conversation
| /// </summary> | ||
| public class CallForwardingExpressionVisitor : ExpressionVisitor | ||
| { | ||
| private static readonly IReadOnlyDictionary<MethodInfo, MethodInfo> _forwardedMethods = new Dictionary<MethodInfo, MethodInfo> |
There was a problem hiding this comment.
Note, I didn't include any methods with ValuTuple, IntPtr, or out since it seems unlikely that a provider will ever support those.
| @@ -47,12 +47,8 @@ public class SqliteMathTranslator : IMethodCallTranslator | |||
| { typeof(Math).GetMethod(nameof(Math.Sign), new[] { typeof(long) })!, "sign" }, | |||
| { typeof(Math).GetMethod(nameof(Math.Sign), new[] { typeof(sbyte) })!, "sign" }, | |||
| { typeof(Math).GetMethod(nameof(Math.Sign), new[] { typeof(short) })!, "sign" }, | |||
| { typeof(MathF).GetMethod(nameof(MathF.Abs), new[] { typeof(float) })!, "abs" }, | |||
| { typeof(MathF).GetMethod(nameof(MathF.Max), new[] { typeof(float), typeof(float) })!, "max" }, | |||
| { typeof(MathF).GetMethod(nameof(MathF.Min), new[] { typeof(float), typeof(float) })!, "min" }, | |||
There was a problem hiding this comment.
These are now redirected to the float overloads on Math. In the end, they still get translated.
| { typeof(double).GetRuntimeMethod(nameof(double.Exp), new[] { typeof(double) })!, typeof(Math).GetRuntimeMethod(nameof(Math.Exp), new[] { typeof(double) })! }, | ||
| { typeof(double).GetRuntimeMethod(nameof(double.Floor), new[] { typeof(double) })!, typeof(Math).GetRuntimeMethod(nameof(Math.Floor), new[] { typeof(double) })! }, | ||
| { typeof(double).GetRuntimeMethod(nameof(double.FusedMultiplyAdd), new[] { typeof(double), typeof(double), typeof(double) })!, typeof(Math).GetRuntimeMethod(nameof(Math.FusedMultiplyAdd), new[] { typeof(double), typeof(double), typeof(double) })! }, | ||
| { typeof(double).GetRuntimeMethod(nameof(double.Ieee754Remainder), new[] { typeof(double), typeof(double) })!, typeof(Math).GetRuntimeMethod(nameof(Math.IEEERemainder), new[] { typeof(double), typeof(double) })! }, |
There was a problem hiding this comment.
We are missing tests for many of these
There was a problem hiding this comment.
Not all of them are actually translated. Should I test all the ones that are on SQL Server? We get coverage that this works by the MathF tests for the methods removed from the translators.
Adding a unit tests felt a bit silly since it's really just a dictionary lookup.
There was a problem hiding this comment.
Note that Math and MathF are no longer evolving so this list won't change. New methods will be added using the generic math pattern with no corresponding Math method. Providers will need to translate those directly (as I did in PR #31301)
Resolves #30916