-
Notifications
You must be signed in to change notification settings - Fork 468
Parse double precision using InvariantCulture #2722
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
base: main
Are you sure you want to change the base?
Changes from all commits
9cc5620
e284fbb
5b53e6c
2b423b8
76528a6
77be4e0
2c03b7f
5cf47de
6fa02d3
62fa208
de50015
86bdd7d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,5 @@ | ||
| using System.Collections.ObjectModel; | ||
| using System.Diagnostics; | ||
| using System.Diagnostics.CodeAnalysis; | ||
| using System.Globalization; | ||
| using System.Text.RegularExpressions; | ||
| using CommunityToolkit.Maui.Core; | ||
|
|
||
|
|
@@ -16,9 +15,11 @@ sealed record MathToken(MathTokenType Type, string Text, object? Value); | |
|
|
||
| sealed partial class MathExpression | ||
| { | ||
| CultureInfo culture { get; init; } = CultureInfo.InvariantCulture; | ||
|
|
||
| readonly IReadOnlyList<MathOperator> operators; | ||
|
|
||
| internal MathExpression(in string expression, in IReadOnlyList<object?> arguments) | ||
| internal MathExpression(in string expression, in IReadOnlyList<object?> arguments, CultureInfo culture) | ||
| { | ||
| ArgumentException.ThrowIfNullOrEmpty(expression, "Expression can't be null or empty."); | ||
| ArgumentNullException.ThrowIfNull(arguments, "Arguments cannot be null."); | ||
|
|
@@ -27,56 +28,56 @@ internal MathExpression(in string expression, in IReadOnlyList<object?> argument | |
|
|
||
| List<MathOperator> operators = | ||
| [ | ||
| new ("+", 2, x => Convert.ToDouble(x[0]) + Convert.ToDouble(x[1])), | ||
| new ("-", 2, x => Convert.ToDouble(x[0]) - Convert.ToDouble(x[1])), | ||
| new ("*", 2, x => Convert.ToDouble(x[0]) * Convert.ToDouble(x[1])), | ||
| new ("/", 2, x => Convert.ToDouble(x[0]) / Convert.ToDouble(x[1])), | ||
| new ("%", 2, x => Convert.ToDouble(x[0]) % Convert.ToDouble(x[1])), | ||
| new ("+", 2, x => ConvertToDouble(x[0]) + ConvertToDouble(x[1])), | ||
| new ("-", 2, x => ConvertToDouble(x[0]) - ConvertToDouble(x[1])), | ||
| new ("*", 2, x => ConvertToDouble(x[0]) * ConvertToDouble(x[1])), | ||
| new ("/", 2, x => ConvertToDouble(x[0]) / ConvertToDouble(x[1])), | ||
| new ("%", 2, x => ConvertToDouble(x[0]) % ConvertToDouble(x[1])), | ||
|
|
||
| new ("and", 2, x => ConvertToBoolean(x[0]) ? x[1] : x[0]), | ||
| new ("or", 2, x => ConvertToBoolean(x[0]) ? x[0] : x[1]), | ||
|
|
||
| new ("==", 2, x => object.Equals(x[0], x[1])), | ||
| new ("!=", 2, x => !object.Equals(x[0], x[1])), | ||
|
|
||
| new ("ge", 2, x => Convert.ToDouble(x[0]) >= Convert.ToDouble(x[1])), | ||
| new ("gt", 2, x => Convert.ToDouble(x[0]) > Convert.ToDouble(x[1])), | ||
| new ("le", 2, x => Convert.ToDouble(x[0]) <= Convert.ToDouble(x[1])), | ||
| new ("lt", 2, x => Convert.ToDouble(x[0]) < Convert.ToDouble(x[1])), | ||
| new ("neg", 1, x => -Convert.ToDouble(x[0])), | ||
| new ("ge", 2, x => ConvertToDouble(x[0]) >= ConvertToDouble(x[1])), | ||
| new ("gt", 2, x => ConvertToDouble(x[0]) > ConvertToDouble(x[1])), | ||
| new ("le", 2, x => ConvertToDouble(x[0]) <= ConvertToDouble(x[1])), | ||
| new ("lt", 2, x => ConvertToDouble(x[0]) < ConvertToDouble(x[1])), | ||
| new ("neg", 1, x => -ConvertToDouble(x[0])), | ||
| new ("not", 1, x => !ConvertToBoolean(x[0])), | ||
| new ("if", 3, x => ConvertToBoolean(x[0]) ? x[1] : x[2]), | ||
|
|
||
| new ("abs", 1, x => Math.Abs(Convert.ToDouble(x[0]))), | ||
| new ("acos", 1, x => Math.Acos(Convert.ToDouble(x[0]))), | ||
| new ("asin", 1, x => Math.Asin(Convert.ToDouble(x[0]))), | ||
| new ("atan", 1, x => Math.Atan(Convert.ToDouble(x[0]))), | ||
| new ("atan2", 2, x => Math.Atan2(Convert.ToDouble(x[0]), Convert.ToDouble(x[1]))), | ||
| new ("ceiling", 1, x => Math.Ceiling(Convert.ToDouble(x[0]))), | ||
| new ("cos", 1, x => Math.Cos(Convert.ToDouble(x[0]))), | ||
| new ("cosh", 1, x => Math.Cosh(Convert.ToDouble(x[0]))), | ||
| new ("exp", 1, x => Math.Exp(Convert.ToDouble(x[0]))), | ||
| new ("floor", 1, x => Math.Floor(Convert.ToDouble(x[0]))), | ||
| new ("ieeeremainder", 2, x => Math.IEEERemainder(Convert.ToDouble(x[0]), Convert.ToDouble(x[1]))), | ||
| new ("log", 2, x => Math.Log(Convert.ToDouble(x[0]), Convert.ToDouble(x[1]))), | ||
| new ("log10", 1, x => Math.Log10(Convert.ToDouble(x[0]))), | ||
| new ("max", 2, x => Math.Max(Convert.ToDouble(x[0]), Convert.ToDouble(x[1]))), | ||
| new ("min", 2, x => Math.Min(Convert.ToDouble(x[0]), Convert.ToDouble(x[1]))), | ||
| new ("pow", 2, x => Math.Pow(Convert.ToDouble(x[0]), Convert.ToDouble(x[1]))), | ||
| new ("round", 2, x => Math.Round(Convert.ToDouble(x[0]), Convert.ToInt32(x[1]))), | ||
| new ("sign", 1, x => Math.Sign(Convert.ToDouble(x[0]))), | ||
| new ("sin", 1, x => Math.Sin(Convert.ToDouble(x[0]))), | ||
| new ("sinh", 1, x => Math.Sinh(Convert.ToDouble(x[0]))), | ||
| new ("sqrt", 1, x => Math.Sqrt(Convert.ToDouble(x[0]))), | ||
| new ("tan", 1, x => Math.Tan(Convert.ToDouble(x[0]))), | ||
| new ("tanh", 1, x => Math.Tanh(Convert.ToDouble(x[0]))), | ||
| new ("truncate", 1, x => Math.Truncate(Convert.ToDouble(x[0]))), | ||
| new ("int", 1, x => Convert.ToInt32(x[0])), | ||
| new ("double", 1, x => Convert.ToDouble(x[0])), | ||
| new ("bool", 1, x => Convert.ToBoolean(x[0])), | ||
| new ("str", 1, x => x[0]?.ToString()), | ||
| new ("len", 1, x => x[0]?.ToString()?.Length), | ||
| new ("^", 2, x => Math.Pow(Convert.ToDouble(x[0]), Convert.ToDouble(x[1]))), | ||
| new ("abs", 1, x => Math.Abs(ConvertToDouble(x[0]))), | ||
| new ("acos", 1, x => Math.Acos(ConvertToDouble(x[0]))), | ||
| new ("asin", 1, x => Math.Asin(ConvertToDouble(x[0]))), | ||
| new ("atan", 1, x => Math.Atan(ConvertToDouble(x[0]))), | ||
| new ("atan2", 2, x => Math.Atan2(ConvertToDouble(x[0]), ConvertToDouble(x[1]))), | ||
| new ("ceiling", 1, x => Math.Ceiling(ConvertToDouble(x[0]))), | ||
| new ("cos", 1, x => Math.Cos(ConvertToDouble(x[0]))), | ||
| new ("cosh", 1, x => Math.Cosh(ConvertToDouble(x[0]))), | ||
| new ("exp", 1, x => Math.Exp(ConvertToDouble(x[0]))), | ||
| new ("floor", 1, x => Math.Floor(ConvertToDouble(x[0]))), | ||
| new ("ieeeremainder", 2, x => Math.IEEERemainder(ConvertToDouble(x[0]), ConvertToDouble(x[1]))), | ||
| new ("log", 2, x => Math.Log(ConvertToDouble(x[0]), ConvertToDouble(x[1]))), | ||
| new ("log10", 1, x => Math.Log10(ConvertToDouble(x[0]))), | ||
| new ("max", 2, x => Math.Max(ConvertToDouble(x[0]), ConvertToDouble(x[1]))), | ||
| new ("min", 2, x => Math.Min(ConvertToDouble(x[0]), ConvertToDouble(x[1]))), | ||
| new ("pow", 2, x => Math.Pow(ConvertToDouble(x[0]), ConvertToDouble(x[1]))), | ||
| new ("round", 2, x => Math.Round(ConvertToDouble(x[0]), ConvertToInt32(x[1]))), | ||
| new ("sign", 1, x => Math.Sign(ConvertToDouble(x[0]))), | ||
| new ("sin", 1, x => Math.Sin(ConvertToDouble(x[0]))), | ||
| new ("sinh", 1, x => Math.Sinh(ConvertToDouble(x[0]))), | ||
| new ("sqrt", 1, x => Math.Sqrt(ConvertToDouble(x[0]))), | ||
| new ("tan", 1, x => Math.Tan(ConvertToDouble(x[0]))), | ||
| new ("tanh", 1, x => Math.Tanh(ConvertToDouble(x[0]))), | ||
| new ("truncate", 1, x => Math.Truncate(ConvertToDouble(x[0]))), | ||
| new ("int", 1, x => ConvertToInt32(x[0])), | ||
| new ("double", 1, x => ConvertToDouble(x[0])), | ||
| new ("bool", 1, x => ConvertToBoolean(x[0])), | ||
| new ("str", 1, x => ConvertToString(x[0])), | ||
| new ("len", 1, x => ConvertToString(x[0])?.Length), | ||
| new ("^", 2, x => Math.Pow(ConvertToDouble(x[0]), ConvertToDouble(x[1]))), | ||
| new ("pi", 0, _ => Math.PI), | ||
| new ("e", 0, _ => Math.E), | ||
| new ("true", 0, _ => true), | ||
|
|
@@ -97,6 +98,7 @@ internal MathExpression(in string expression, in IReadOnlyList<object?> argument | |
| } | ||
|
|
||
| this.operators = operators; | ||
| this.culture = culture; | ||
| } | ||
|
|
||
| static ReadOnlyDictionary<string, string> BinaryMappingDictionary { get; } = new Dictionary<string, string> | ||
|
|
@@ -224,7 +226,7 @@ internal MathExpression(in string expression, in IReadOnlyList<object?> argument | |
| [GeneratedRegex("""^(\-|\!)""")] | ||
| private static partial Regex EvaluateUnaryOperators(); | ||
|
|
||
| [GeneratedRegex("""^(\-?\d+\.\d+|\-?\d+)""")] | ||
| [GeneratedRegex("""^(\-?\d+[\.,]\d+|\-?\d+)""")] | ||
| private static partial Regex EvaluateNumberPattern(); | ||
|
|
||
| [GeneratedRegex("""^["]([^"]*)["]""")] | ||
|
|
@@ -242,15 +244,21 @@ internal MathExpression(in string expression, in IReadOnlyList<object?> argument | |
| [GeneratedRegex("""^\s*""")] | ||
| private static partial Regex EvaluateWhitespace(); | ||
|
|
||
| static bool ConvertToBoolean(object? b) => b switch | ||
| bool ConvertToBoolean(object? b) => b switch | ||
| { | ||
| bool x => x, | ||
| null => false, | ||
| double doubleValue => doubleValue != 0 && !double.IsNaN(doubleValue), | ||
| string stringValue => !string.IsNullOrEmpty(stringValue), | ||
| _ => Convert.ToBoolean(b) | ||
| _ => Convert.ToBoolean(b, culture) | ||
| }; | ||
|
|
||
| double ConvertToDouble(object? x) => Convert.ToDouble(x, culture); | ||
|
|
||
| int ConvertToInt32(object? x) => Convert.ToInt32(x, culture); | ||
|
|
||
| string? ConvertToString(object? x) => Convert.ToString(x, culture); | ||
|
|
||
| bool ParsePattern(Regex regex) | ||
| { | ||
| var whitespaceMatch = EvaluateWhitespace().Match(Expression[ExpressionIndex..]); | ||
|
|
@@ -362,7 +370,7 @@ bool ParsePrimary() | |
| if (ParsePattern(EvaluateNumberPattern())) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be good to turn on the CA1305 analyzer in the project to avoid similar bugs.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @MartyIX based on your feedback, I applied the CA1305 analyzer and found multiple conversions in MathExpression.shared.cs that needed updates to satisfy the analyzer. After the changes all unit tests for the math converters still passed. The CA1305 analyzer also picked up new issues which isn't part of the scope of this PR. Would you like me to create split 3 issues to look? (Camera CA1305, SpeechToText CA1305, Badge CA1305)? Build failed with 7 error(s) and 81 warning(s) in 3.8s
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can change the pattern to something like this: It should match:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @VladislavAntonyuk, sorry it's been awhile since I've replied to your comment. The struggle I have with your suggestion is that we're trying to make the expressions follow InvariantCulture so that the expressions are deterministic and will not be influence by language changes. The other reason is that the comma (,) is already part of the expression for delimiting functional arguments (e.g. atan2).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @VladislavAntonyuk, I watched this month's standup and noted two key discussions:
Just to reiterate my application's use case: the application supports runtime switching between 30+ languages, which updates CultureInfo.CurrentCulture and CultureInfo.CurrentUICulture. However, our UI uses MultiMathExpressionConverter for some spinner logic (e.g., x0 ? -90 : 0) which needs to be language agnostic. In Arabic, parsing the negative sign causes a crash. The crash can be understood because of Arabic not recognizing the minus sign. CultureInfo.CurrentCulture = new CultureInfo("ar-AR");
var value = double.Parse("-90"); // Throws System.FormatException: 'The input string '-90' was not in a correct format.'In our application, we have worked around this problem by coming up with alternates ways to specify -90. |
||
| { | ||
| string _number = PatternMatch.Groups[1].Value; | ||
| RPN.Add(new MathToken(MathTokenType.Value, _number, double.Parse(_number))); | ||
| RPN.Add(new MathToken(MathTokenType.Value, _number, double.Parse(_number, culture))); | ||
| return true; | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.