View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0031176 | FPC | RTL | public | 2016-12-30 16:03 | 2017-01-25 22:24 |
Reporter | Dmitriy Pomerantsev | Assigned To | Michael Van Canneyt | ||
Priority | normal | Severity | minor | Reproducibility | always |
Status | resolved | Resolution | fixed | ||
Product Version | 3.1.1 | Product Build | 35216 | ||
Target Version | 3.2.0 | Fixed in Version | 3.1.1 | ||
Summary | 0031176: [patch] SysUtils.StrToBool() may fail with non latin locales | ||||
Description | If a programmer is trying to convert a string into a Boolean value with a floating point in a locale where the decimal separator is ",", the function will raise an exception. This is because inside is used the Val() procedure, which suports only the "." decimal separator. Fortunately in functions such TryStrToFloat(), this problem has been solved. I propose a patch to resolve this issue. | ||||
Steps To Reproduce | The sample program: {$MODE OBJFPC}{$H+} program strtobool; uses SysUtils; begin FormatSettings.DecimalSeparator := ','; Writeln(SysUtils.StrToBool('1,0')); end. Result in current FPC version: An unhandled exception occurred at $0000000100014350: EConvertError: "1,0" is not a valid boolean. $0000000100014350 $00000001000016BD main, line 8 of strtobool.lpr $00000001000016F6 $000000010000DFD3 $0000000100001682 $0000000076FB59CD $00000000770EA561 Result with patch: TRUE | ||||
Tags | No tags attached. | ||||
Fixed in Revision | 35331 | ||||
FPCOldBugId | |||||
FPCTarget | |||||
Attached Files |
|
|
TryStrToBool.patch (1,026 bytes)
Index: rtl/objpas/sysutils/sysstr.inc =================================================================== --- rtl/objpas/sysutils/sysstr.inc (revision 35216) +++ rtl/objpas/sysutils/sysstr.inc (working copy) @@ -1969,14 +1969,17 @@ Code: word; begin Temp:=upcase(S); - Val(temp,D,code); - Result:=true; - If Code=0 then {$ifdef FPUNONE} - Value:=(D<>0) + Result:=TryStrToInt(Temp,D); {$else} - Value:=(D<>0.0) + Result:=TryStrToFloat(Temp,D); {$endif} + If Result then +{$ifdef FPUNONE} + Value:=(D<>0) +{$else} + Value:=(D<>0.0) +{$endif} else begin CheckBoolStrs; @@ -1984,13 +1987,13 @@ if Temp=upcase(TrueBoolStrs[I]) then begin Value:=true; - exit; + exit(true); end; for I:=low(FalseBoolStrs) to High(FalseBoolStrs) do if Temp=upcase(FalseBoolStrs[I]) then begin Value:=false; - exit; + exit(true); end; Result:=false; end; |
|
IMHO your patch breaks backwards compatibility. FormatSettings.DecimalSeparator:=','; B:=StrToBool('1.0') ; will currently set B to true. With your patch, it will set it to False (or worse, raise an exception) because 1.0 is not a valid float. |
|
And Delphi agree with you. If a decimal separator is a comma, the "1.0" is not a valid number. (We will get an exception.) If you only knew how many programmers used locale depended functions, and then wondered - why the program stopped working outside of Russia. ;-) But this is the right way. For special cases, there are overloaded functions that accept TFormatSettings as a parameter. But not for StrToBool(). At least now. |
|
So what's next? I understand that Free Pascal's RTL sometimes gently expand Delphi RTL. For example, even in 64-bit Delphi you can't have a string longer than 2Gb, arrays of more High(Integer) elements, etc. Maybe StrToBool() can support the floats with a dot in any locale. But it's not its job. All RTL functions must comply the same rules. All related functions are depends on FormatSettings. And once StrToBool() delegates string to float conversion to TryStrToFloat(), then TryStrToFloat() have to decide what the float number and what's not. Because every function have to do its job. And nothing else. Perhaps, in this case is better to write in fpc-devel and ask the opinion of other developers. |
|
Wouldn't it then falsely interpret 1.000 (one thousand with a thousandseparator) as 1.0 ? |
|
*sigh* So now we have a Torvalds memcpy() situation. Now StrToBool() doesn't work properly. You afraid that the correct fix break programs that rely on the current behavior. But the current or fixed behavior can break the correct programs. For example with decimal separator "," and assuming "." is also decimal separator, we get False with "." or even "0." string as input. But it can be just a random strings, not a false booleans. Error may occur much later. But the way, your example is incorrect because ThousandSeparator is for Currency type and FormatSettings.ThousandSeparator := '.'; FormatSettings.DecimalSeparator := ','; StrToFloat('1' + FormatSettings.ThousandSeparator + '000') gives us "EConvertError: '1.000' is not a valid floating point value" both in Delphi and FPC. StrToBool('1' + FormatSettings.ThousandSeparator + '000') gives us EConvertError: '1.000' is not a valid boolean value in Delphi. |
|
StrToBool now also accepts a localized float. The non-localized version takes priority. It means there is now an overloaded version that accepts a TFormatSettings argument for StrToBool, TryStrToBool, StrToBoolDef. |
Date Modified | Username | Field | Change |
---|---|---|---|
2016-12-30 16:03 | Dmitriy Pomerantsev | New Issue | |
2016-12-30 16:03 | Dmitriy Pomerantsev | File Added: TryStrToBool.patch | |
2016-12-30 16:45 | Michael Van Canneyt | Assigned To | => Michael Van Canneyt |
2016-12-30 16:45 | Michael Van Canneyt | Status | new => assigned |
2016-12-30 16:47 | Michael Van Canneyt | Note Added: 0097174 | |
2016-12-30 16:47 | Michael Van Canneyt | Status | assigned => feedback |
2016-12-30 16:47 | Michael Van Canneyt | Note Edited: 0097174 | View Revisions |
2016-12-30 19:53 | Dmitriy Pomerantsev | Note Added: 0097176 | |
2016-12-30 19:53 | Dmitriy Pomerantsev | Status | feedback => assigned |
2016-12-30 19:53 | Dmitriy Pomerantsev | Note Edited: 0097176 | View Revisions |
2017-01-11 14:07 | Dmitriy Pomerantsev | Note Added: 0097413 | |
2017-01-12 10:26 | Marco van de Voort | Note Added: 0097430 | |
2017-01-12 12:04 | Dmitriy Pomerantsev | Note Added: 0097433 | |
2017-01-12 12:05 | Dmitriy Pomerantsev | Note Edited: 0097433 | View Revisions |
2017-01-12 12:05 | Dmitriy Pomerantsev | Note Edited: 0097433 | View Revisions |
2017-01-12 12:06 | Dmitriy Pomerantsev | Note Edited: 0097433 | View Revisions |
2017-01-12 12:11 | Dmitriy Pomerantsev | Note Edited: 0097433 | View Revisions |
2017-01-25 22:24 | Michael Van Canneyt | Fixed in Revision | => 35331 |
2017-01-25 22:24 | Michael Van Canneyt | Note Added: 0097708 | |
2017-01-25 22:24 | Michael Van Canneyt | Status | assigned => resolved |
2017-01-25 22:24 | Michael Van Canneyt | Fixed in Version | => 3.1.1 |
2017-01-25 22:24 | Michael Van Canneyt | Resolution | open => fixed |
2017-01-25 22:24 | Michael Van Canneyt | Target Version | => 3.2.0 |