Re: [PATCH 1/5] xen/lib: Move simple_strtoul from common/vsprintf.c to lib

[Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>]

On 7/31/23 10:52 AM, Jan Beulich wrote:
> On 28.07.2023 23:35, Shawn Anastasio wrote:
>> Move the simple_strtoul routine which is used throughout the codebase
>> from vsprintf.c to its own file in xen/lib.
>>
>> This allows libfdt to be built on ppc64 even though xen/common doesn't
>> build yet.
>>
>> Signed-off-by: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
>> ---
>>  xen/common/vsprintf.c    | 37 -------------------------------------
>>  xen/lib/Makefile         |  1 +
>>  xen/lib/simple_strtoul.c | 40 ++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 41 insertions(+), 37 deletions(-)
>>  create mode 100644 xen/lib/simple_strtoul.c
> 
> What about its siblings? It'll be irritating to find one here and the
> other there.

I was debating whether to do this or not and ultimately decided to only
make the minimum changes that were required right now. I can go ahead
and make the change for its siblings as well.

> Also please no underscores in (new) filenames unless there's a reason
> for this. In the case here, though, I question the need for "simple"
> in the file name in the first place.

>From a look at the other files in xen/lib there seemed to be a
convention of naming files after the exact function they implement.
Would you rather I rename it to just strtoul.c? Or simple-strotoul.c?

>> --- /dev/null
>> +++ b/xen/lib/simple_strtoul.c
>> @@ -0,0 +1,40 @@
>> +/*
>> + *  Copyright (C) 1991, 1992  Linus Torvalds
>> + */
>> +
>> +#include <xen/ctype.h>
>> +
>> +/**
>> + * simple_strtoul - convert a string to an unsigned long
>> + * @cp: The start of the string
>> + * @endp: A pointer to the end of the parsed string will be placed here
>> + * @base: The number base to use
>> + */
>> +unsigned long simple_strtoul(
>> +    const char *cp, const char **endp, unsigned int base)
>> +{
>> +    unsigned long result = 0,value;
>> +
>> +    if (!base) {
>> +        base = 10;
>> +        if (*cp == '0') {
>> +            base = 8;
>> +            cp++;
>> +            if ((toupper(*cp) == 'X') && isxdigit(cp[1])) {
>> +                cp++;
>> +                base = 16;
>> +            }
>> +        }
>> +    } else if (base == 16) {
>> +        if (cp[0] == '0' && toupper(cp[1]) == 'X')
>> +            cp += 2;
>> +    }
>> +    while (isxdigit(*cp) &&
>> +           (value = isdigit(*cp) ? *cp-'0' : toupper(*cp)-'A'+10) < base) {
>> +        result = result*base + value;
>> +        cp++;
>> +    }
>> +    if (endp)
>> +        *endp = cp;
>> +    return result;
>> +}
> 
> While moving, I think it would be nice if this stopped using neither
> Xen nor Linux style. I'm not going to insist, but doing such style
> adjustments right here would be quite nice.

Especially if I'm going to be moving its siblings, I'd rather just copy
the functions verbatim for this patch, if that's acceptable.

> Jan

Thanks,
Shawn