TFormula: support locale settings for which the decimal separator is "," #17327
Conversation
lmoneta
left a comment
lmoneta
left a comment
There was a problem hiding this comment.
LGTM, if it passes the tests should be fine.
Thank you Danilo for the fix
| TString value = TString::Format("%lf", (*constIt).second); | ||
| // #17225: we take into account LC_LOCALE settings for which the decimal separator | ||
| // is , instead of ., e.g. de_AT.UTF-8 | ||
| TString value = TString::Format("%lf", (*constIt).second).ReplaceAll(",", "."); |
There was a problem hiding this comment.
Shouldn't this be dependent on the value of LC_LOCALE? (in some local we might have 1,200.00 for one thousand two hundreds).
There was a problem hiding this comment.
it's a good observation. But how can we adapt to all possible locale?
There was a problem hiding this comment.
actually it seems that there are only 3 decimal separators: comma, period and arabic decimal separator (https://randombits.dev/articles/number-localization/locale-list)
There was a problem hiding this comment.
Thinking more about it... If we have an arabic decimal separator, it means that we also have arabic digits. At that point, I am unsure whether the decimal separator would be the first problem we hit...
There was a problem hiding this comment.
Actually, I think we need a larger conversation about local and how to handle them. It seems odds/confusion to all support local for this single functions. Until we have this conversation, I recommend we issue an explicit error when encountering a comma here.
Test Results 18 files 18 suites 4d 4h 24m 48s ⏱️ For more details on these failures, see this check. Results for commit c7a2716. ♻️ This comment has been updated with latest results. |
In root-project#18194, an optional parameter to `TFormula::GetExpFormula()` was introduced, which can be used to customize the precision when putting parameters into the jitted code. However, I don't see the reason why wouldn't like to always print the floating point numbers with maximum precision, which is suggested in this commit. This is achieved using modern C++ functions, which are also not respecting the users locale setting, but instead use the classic C locale by default, which is what we need when generating code. Follows up on root-project#18194 (and its partial revert root-project#18216). Closes root-project#17225. Replaces root-project#17327.
|
Superseded by #19734. |
In #18194, an optional parameter to `TFormula::GetExpFormula()` was introduced, which can be used to customize the precision when putting parameters into the jitted code. However, I don't see the reason why wouldn't like to always print the floating point numbers with maximum precision, which is suggested in this commit. This is achieved using modern C++ functions, which are also not respecting the users locale setting, but instead use the classic C locale by default, which is what we need when generating code. Follows up on #18194 (and its partial revert #18216). Closes #17225. Replaces #17327.
3 participants
Fixes #17225