[PATCH] kstrtox: reuse functions from ctype.h

[PATCH] kstrtox: reuse functions from ctype.h

[Michal Nazarewicz]

kstrto*() family of functions uses open coded test
for a hexadecimal digit and own implementation of
tolower() function.  This commit changes the code
to use definitions from ctype.h.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 lib/kstrtox.c |   13 ++++---------
 1 files changed, 4 insertions(+), 9 deletions(-)

Was there a reason not to use code from ctype.h?

I know that ctype.h's tolower() is kinda creepy but
IMO it's still better to reuse.

diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index 05672e8..ba6c0d5 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -18,11 +18,6 @@
 #include <linux/module.h>
 #include <linux/types.h>
 
-static inline char _tolower(const char c)
-{
-	return c | 0x20;
-}
-
 static int _kstrtoull(const char *s, unsigned int base, unsigned long long *res)
 {
 	unsigned long long acc;
@@ -30,14 +25,14 @@ static int _kstrtoull(const char *s, unsigned int base, unsigned long long *res)
 
 	if (base == 0) {
 		if (s[0] == '0') {
-			if (_tolower(s[1]) == 'x' && isxdigit(s[2]))
+			if (tolower(s[1]) == 'x' && isxdigit(s[2]))
 				base = 16;
 			else
 				base = 8;
 		} else
 			base = 10;
 	}
-	if (base == 16 && s[0] == '0' && _tolower(s[1]) == 'x')
+	if (base == 16 && s[0] == '0' && tolower(s[1]) == 'x')
 		s += 2;
 
 	acc = 0;
@@ -47,8 +42,8 @@ static int _kstrtoull(const char *s, unsigned int base, unsigned long long *res)
 
 		if ('0' <= *s && *s <= '9')
 			val = *s - '0';
-		else if ('a' <= _tolower(*s) && _tolower(*s) <= 'f')
-			val = _tolower(*s) - 'a' + 10;
+		else if (isxdigit(*s))
+			val = tolower(*s) - 'a' + 10;
 		else if (*s == '\n') {
 			if (*(s + 1) == '\0')
 				break;
-- 
1.7.3.1

Re: [PATCH] kstrtox: reuse functions from ctype.h

[Alexey Dobriyan]

On Thu, Apr 14, 2011 at 4:34 PM, Michal Nazarewicz <mina86@mina86.com> wrote:
> kstrto*() family of functions uses open coded test
> for a hexadecimal digit and

Yes, so?

> own implementation of tolower() function.

No!
It's special cased for this very usage, because the rest of ASCII is
of no concern.
It doesn't claim tolower() semantics.

> -                       if (_tolower(s[1]) == 'x' && isxdigit(s[2]))
> +                       if (tolower(s[1]) == 'x' && isxdigit(s[2]))
>                                base = 16;
>                        else
>                                base = 8;
>                } else
>                        base = 10;
>        }
> -       if (base == 16 && s[0] == '0' && _tolower(s[1]) == 'x')
> +       if (base == 16 && s[0] == '0' && tolower(s[1]) == 'x')
>                s += 2;
>
>        acc = 0;
> @@ -47,8 +42,8 @@ static int _kstrtoull(const char *s, unsigned int base, unsigned long long *res)
>
>                if ('0' <= *s && *s <= '9')
>                        val = *s - '0';
> -               else if ('a' <= _tolower(*s) && _tolower(*s) <= 'f')
> -                       val = _tolower(*s) - 'a' + 10;
> +               else if (isxdigit(*s))

[0-9] are isxdigit() as well, so the code sort of logically duplicate.

> +                       val = tolower(*s) - 'a' + 10;

Re: [PATCH] kstrtox: reuse functions from ctype.h

[Michal Nazarewicz]

On Thu, 14 Apr 2011 16:06:34 +0200, Alexey Dobriyan <adobriyan@gmail.com>  
wrote:

> On Thu, Apr 14, 2011 at 4:34 PM, Michal Nazarewicz <mina86@mina86.com>  
> wrote:
>> kstrto*() family of functions uses open coded test
>> for a hexadecimal digit and
>
> Yes, so?

So it's better to avoid open-coded code without a reason if there is
already function for what one tries to do.

>> own implementation of tolower() function.
>
> No!
> It's special cased for this very usage, because the rest of ASCII is
> of no concern.
> It doesn't claim tolower() semantics.

Either way, the question is do we care about this optimisation so much
as to use it and somehow duplicate code which already is in ctype.h.

I'm not saying we don't.  I'm asking whether we want to.

>> @@ -47,8 +42,8 @@ static int _kstrtoull(const char *s, unsigned int  
>> base, unsigned long long *res)
>>
>>                if ('0' <= *s && *s <= '9')
>>                        val = *s - '0';
>> -               else if ('a' <= _tolower(*s) && _tolower(*s) <= 'f')
>> -                       val = _tolower(*s) - 'a' + 10;
>> +               else if (isxdigit(*s))
>
> [0-9] are isxdigit() as well, so the code sort of logically duplicate.

Yes, so? ;)

I think isxdigit(*s) looks nicer than “'a' <= _tolower(*s) &&
_tolower(*s) <= 'f'”.

>> +                       val = tolower(*s) - 'a' + 10;

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michal "mina86" Nazarewicz    (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--

Re: [PATCH] kstrtox: reuse functions from ctype.h

[Alexey Dobriyan]

On Thu, Apr 14, 2011 at 04:30:17PM +0200, Michal Nazarewicz wrote:
> On Thu, 14 Apr 2011 16:06:34 +0200, Alexey Dobriyan <adobriyan@gmail.com>  
> wrote:

> >> @@ -47,8 +42,8 @@ static int _kstrtoull(const char *s, unsigned int  
> >> base, unsigned long long *res)
> >>
> >>                if ('0' <= *s && *s <= '9')
> >>                        val = *s - '0';
> >> -               else if ('a' <= _tolower(*s) && _tolower(*s) <= 'f')
> >> -                       val = _tolower(*s) - 'a' + 10;
> >> +               else if (isxdigit(*s))
> >
> > [0-9] are isxdigit() as well, so the code sort of logically duplicate.
> 
> Yes, so? ;)
> 
> I think isxdigit(*s) looks nicer than “'a' <= _tolower(*s) &&
> _tolower(*s) <= 'f'”.

If you write isxdigit(*s) you have to write some other expression for
"val = " to be nicer.

The point it's doesn't matter, compiler will optimize all those _tolower().