Hi, On Mon, 8 Jun 2020 at 00:24, Michal Simek <[email protected]> wrote: > > 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.
Once this revert is in I think we should refrain from changing it until there are tests to cover its current behaviour, e.g. in str_ut.c Reviewed-by: Simon Glass <[email protected]> Regards, Simon
