On 6/7/20 2:40 AM, Heinrich Schuchardt wrote:
> On 6/7/20 7:36 AM, Sean Anderson wrote:
>> This reverts commit 0486497e2b5f4d36fa968a1a60fea358cbf70b65.
>>
>> The strtoul has well-defined semantics. It is defined by the C standard and
>> POSIX. To quote the relevant section of the man pages,
>>
>>> If base is zero or 16, the string may then include a "0x" prefix, and the
>>> number will be read in base 16; otherwise, a zero base is taken as 10
>>> (decimal) unless the next character is '0', in which case it is taken as
>>> 8 (octal).
>>
>> Keeping these semantics is important for several reasons. First, it is very
>> surprising for standard library functions to behave differently than usual.
>> Every other implementation of strtoul has different semantics than the
>> implementation in U-Boot at the moment. Second, it can result in very
>> surprising results from small changes. For example, changing the string
>> "1f" to "20" causes the parsed value to *decrease*. Forcing use of the "0x"
>> prefix to specify hexidecimal numbers is a feature, not a bug. Lastly, this
>> is slightly less performant, since the entire number is parsed twice.
>>
>> This fixes the str_simple_strtoul test failing with
>>
>> test/str_ut.c:29, run_strtoul(): expect_val == val: Expected 0x44b (1099),
>> got 0x1099ab (1087915)
>> test/str_ut.c:46, str_simple_strtoul(): 0 == run_strtoul(uts, str2, 0, 1099,
>> 4): Expected 0x0 (0), got 0x1 (1)
>>
>
> I suggest to add a Fixes line here:
>
> Fixes: 0486497e2b5f ("lib: Improve _parse_integer_fixup_radix base 16
> detection")
Thanks, I will add that.
>
> Cf.
> https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes
>
>> Signed-off-by: Sean Anderson <sean...@gmail.com>
>> CC: Michal Simek <michal.si...@xilinx.com>
>> CC: Shiril Tichkule <shir...@xilinx.com>
>> ---
>>
>> lib/strto.c | 18 +-----------------
>> 1 file changed, 1 insertion(+), 17 deletions(-)
>>
>> diff --git a/lib/strto.c b/lib/strto.c
>> index 3d77115d4d..c00bb5895d 100644
>> --- a/lib/strto.c
>> +++ b/lib/strto.c
>> @@ -22,25 +22,9 @@ static const char *_parse_integer_fixup_radix(const char
>> *s, unsigned int *base)
>> *base = 16;
>> else
>> *base = 8;
>> - } else {
>> - int i = 0;
>> - char var;
>> -
>> + } else
>
> scripts/checkpatch.pl reports a problem:
>
> CHECK: Unbalanced braces around else statement
> #50: FILE: lib/strto.c:25:
> + } else
I chose this form since that is what is used in Linux, and that is
how it formatted before the commit this reverts. I do think that it is
better form to have braces around this else block, though.
--Sean
