On 07. 06. 20 7:36, 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) > > Signed-off-by: Sean Anderson <[email protected]> > CC: Michal Simek <[email protected]> > CC: Shiril Tichkule <[email protected]> > --- > > 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 > *base = 10; > - > - do { > - var = tolower(s[i++]); > - if (var >= 'a' && var <= 'f') { > - *base = 16; > - break; > - } > - > - if (!(var >= '0' && var <= '9')) > - break; > - } while (var); > - } > } > - > if (*base == 16 && s[0] == '0' && tolower(s[1]) == 'x') > s += 2; > return s; >
It is in u-boot mainline from February. Then we had to fix it in April. In the middle of this I have seen IIC one patchset which improves hex handling which is likely better way then this. I am fine with reverting this patch but I would also like to see more comments in the code to say that we shouldn't really change this. Thanks, Michal
