K&R 2 exercise 2-3

H

Herrcho

Hi~ i've studied C for a few months myself,

and i'd appreciate it if anyone could improve my coding or correct it.

the following is my solution to the K&R exercise 2-3

"Write the function htoi(s), which converts a string of hexademical digits
(including an optional 0x or 0X) into its equivalent integer value.
The allowable digits are 0 through 9, a through f, and A throught F."


//**************************************************************************

#include <stdio.h>

int isxdigit2(int c)
{
if ( (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F') || (c >= '0' && c <= '9') )
return 1;
else
return 0;
}

int tolower2(int c)
{
if (c >= 'A' && c <= 'Z')
return c+32;
else
return c;
}

int power2(int base, int num)
{
int sum;
for (sum = 1; num >0 ; num --)
sum *= base;
return sum;
}

int char_to_num(int c)
{
if (c >= '0' && c <= '9')
{
return c - 48;
}
else
{
return 10 + (tolower2(c) - 'a');
}
}

int htoi(char *c)
{
int i, k, prefix = 0;
size_t sum = 0;

if (c[0] == '0' && tolower2(c[1]) == 'x')
prefix = 1;

for (i = (prefix == 1)? 2:0 ; c ;i++ )
{
if (!isxdigit2(c) )
{
printf("Wrong hexa number\n");
return 0;
}
c = char_to_num(c);
}

for (k = (prefix == 1)? 2 : 0 ; k <= i-1 ; ++k )
{
sum += c[k] * power2(16, i-1-k);
}

return sum;
}

int main()
{
char c[] = "0xAB";
printf("%u", htoi(c));

return 0;
}

//******************************************************************

when i change char c[] to char *c in main(),
it shows error, why ??

Thanks..
 
P

Peter Pichler

Herrcho said:
Hi~ i've studied C for a few months myself,

Good for you.
and i'd appreciate it if anyone could improve my coding or correct it.

the following is my solution to the K&R exercise 2-3

Excellent, finally someone who has actually shown some code! ;-)
Beware, many of my comments below are nits, but it is nice to learn the
good habits now before you'd need to unlearn the wrong ones, like myself.
"Write the function htoi(s), which converts a string of hexademical digits
(including an optional 0x or 0X) into its equivalent integer value.
The allowable digits are 0 through 9, a through f, and A throught F."

//**************************************************************************

#include <stdio.h>

int isxdigit2(int c)

Identifiers starting with 'is' followed by a lowercase letter are reserved
by the C standard. Use something like is_xdigit2 instead.
By the way, there already is a macro isxdigit, you need to #include
{
if ( (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F') || (c >= '0' && c <=
'9') )

This is not guaranteed to work on non-ASCII systems. The ranges 'a'..'f' and
'A'..'F' do not need to be consecutive. It is guaranteed about '0'..'9',
though.
return 1;
else
return 0;
}

Proper indentation would make your code easier to read.
int tolower2(int c)
{
if (c >= 'A' && c <= 'Z')
return c+32;
else
return c;
}

Same comments as above. BTW, identifiers starting with 'to' are reserved
too.
int power2(int base, int num)
{
int sum;
for (sum = 1; num >0 ; num --)
sum *= base;
return sum;
}

int char_to_num(int c)
{
if (c >= '0' && c <= '9')
{
return c - 48;

Where does the magic number 48 come from? ITYM return c - '0';
}
else
{
return 10 + (tolower2(c) - 'a');

Again, this only works if you are sure that 'a'..'f' is a consecutive set.
}
}

int htoi(char *c)
{
int i, k, prefix = 0;
size_t sum = 0;

if (c[0] == '0' && tolower2(c[1]) == 'x')
prefix = 1;

Are you sure that you have 2 valid characters in c? What happens when you
call htoi("7"), for example?
Try:

if (c[0] && c[0] == '0' && c[1] && (c[1] == 'x' || c[1] == 'X'))
prefix = 1;

It's admittably a bit less elegant but safer. By the way, you could work on
an algorithm without the variable prefix. It's really simple, think about it
a little. All you need is to skip the first 2 characters of c...
for (i = (prefix == 1)? 2:0 ; c ;i++ )
{
if (!isxdigit2(c) )
{
printf("Wrong hexa number\n");


"Hexa" as in "jinxed"? ;-)
return 0;
}
c = char_to_num(c);
}

for (k = (prefix == 1)? 2 : 0 ; k <= i-1 ; ++k )
{
sum += c[k] * power2(16, i-1-k);
}

return sum;
}

int main()
{
char c[] = "0xAB";
printf("%u", htoi(c));


Err, no sir. %u in printf() expects unsigned int, but you provide it with
the result of htoi(), which returns int. This is strictly speaking an
undefined behaviour, although admittably I have yet to see a platform where
it does not work. In any case, use %d instead or change your htoi() to
return unsigned int.
return 0;
}

It looks OK, though overly complicated. You could use already available
macros isxdigit and tolower (both defined in ctype.h), but frankly you
should not even need them. And you certainly should not need your power2().

Think about it. What's 1986 in decimal?
In your algorithm, it's 1*10^3 + 9*10^2 + 8*10^1 + 1*10^0.
How about (((1)*10 + 9)*10 + 8)*10 + 6?

Now try converting it to a C program, for any (hexa)decimal number with any
number of digits.
//******************************************************************

when i change char c[] to char *c in main(),
it shows error, why ??

Undefined behaviour. Your htoi() tries to alter the string it is given in
situ. With char c[] in main, the string literal "0xAB" get copied to a local
array c, which is OK. But when you declare c as char *, you pass a pointer
to the string literal to htoi()... oops!

HTH,

Peter
 
N

nrk

Herrcho said:
Hi~ i've studied C for a few months myself,

and i'd appreciate it if anyone could improve my coding or correct it.

the following is my solution to the K&R exercise 2-3

"Write the function htoi(s), which converts a string of hexademical digits
(including an optional 0x or 0X) into its equivalent integer value.
The allowable digits are 0 through 9, a through f, and A throught F."

First off, not a bad attempt. But the biggest problem is that you've
assumed ASCII character set. Read on for the complete review...
//**************************************************************************

#include <stdio.h>

#include said:
int isxdigit2(int c)
{
if ( (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F') || (c >= '0'
&& c <= '9') )

Here you assume 'a'-'f' and 'A'-'F' are in contiguous ascending order in the
character set. This need not be true (yes, even I wasn't happy to know
that). Note however that '0'-'9' are guaranteed to be in contiguous
ascending order by the standard.

So, what do we do? Simple, we keep a string with valid hex-alphabets and
see if the character to be checked occurs within that string:

static const char *hexalpha = "abcdefABCDEF";

if ( (c >= '0' && c <= '9') || (c && strchr(hexalpha, c)) )
return 1;
else
return 0;
}

Alternately, you could just use the isxdigit standard function, but that's
way less fun :)
int tolower2(int c)
{
if (c >= 'A' && c <= 'Z')
return c+32;

Ouch!! Once again, you've not only assumed that 'A'-'Z' are contiguous
ascending, but also assumed that 'A'+32 == 'a', which need not be true at
all. You could either use the standard tolower, or:
static const char *uppercase = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
static const char *lowercase = "abcdefghijklmnopqrstuvwxyz";

char *cptr = strchr(uppercase, c);
if ( cptr ) return lowercase[cptr - uppercase];
return c;
else
return c;
}

int power2(int base, int num)
{
int sum;
for (sum = 1; num >0 ; num --)
sum *= base;
return sum;
}

Hmmm... way too complicated... see below for why this isn't needed.
int char_to_num(int c)
{
if (c >= '0' && c <= '9')
{
return c - 48;

Ouch!! Here you assumed '0' == 48, which need not be true. Why not:
return c - '0';
}
else
{
return 10 + (tolower2(c) - 'a');

Again, 'a'-'f' need not be contiguous ascending in the character set. What
you can do instead is:
static const char *hexalpha = "abcdef";
return 10 + (strchr(hexalpha, tolower2(c)) - hexalpha);
}
}

int htoi(char *c)

Make that:
unsigned int htoi(char *c)
{
int i, k, prefix = 0;
size_t sum = 0; unsigned int sum;

if (c[0] == '0' && tolower2(c[1]) == 'x')
prefix = 1;

for (i = (prefix == 1)? 2:0 ; c ;i++ )
{
if (!isxdigit2(c) )
{
printf("Wrong hexa number\n");
return 0;
}
c = char_to_num(c);
}

for (k = (prefix == 1)? 2 : 0 ; k <= i-1 ; ++k )
{
sum += c[k] * power2(16, i-1-k);
}

return sum;
}


You can simplify matters quite a bit if you recognize that proceeding
left-to-right in the string is the best thing that you can do. Simply
multiply whatever you have by 16 and add the next number in line, and voila
you have the correct conversion at the end.

if ( c[0] == '0' && (c[1] == 'x' || c[1] == 'X') )
c += 2;
while ( *c && isxdigit2(*c) ) {
sum = (sum * 16) + char_to_num(*c);
++c;
}
if ( *c ) {
fprintf(stderr, "Invalid hex digit %c in string\n", *c);
sum = 0;
}
return sum;
int main()
{
char c[] = "0xAB";
printf("%u", htoi(c));

return 0;
}

//******************************************************************

when i change char c[] to char *c in main(),
it shows error, why ??

Who shows what error?

-nrk.
 
N

nrk

Peter said:
Good for you.


Excellent, finally someone who has actually shown some code! ;-)
Beware, many of my comments below are nits, but it is nice to learn the
good habits now before you'd need to unlearn the wrong ones, like myself.

//**************************************************************************

Identifiers starting with 'is' followed by a lowercase letter are reserved
by the C standard. Use something like is_xdigit2 instead.
By the way, there already is a macro isxdigit, you need to #include

'9') )

This is not guaranteed to work on non-ASCII systems. The ranges 'a'..'f'
and 'A'..'F' do not need to be consecutive. It is guaranteed about
'0'..'9', though.


Proper indentation would make your code easier to read.

The indentation's ok, but he/she used tabs instead of spaces. To OP: You
should use spaces to indent code that you post on the usenet.
int tolower2(int c)
{
if (c >= 'A' && c <= 'Z')
return c+32;
else
return c;
}

Same comments as above. BTW, identifiers starting with 'to' are reserved
too.
int power2(int base, int num)
{
int sum;
for (sum = 1; num >0 ; num --)
sum *= base;
return sum;
}

int char_to_num(int c)
{
if (c >= '0' && c <= '9')
{
return c - 48;

Where does the magic number 48 come from? ITYM return c - '0';
}
else
{
return 10 + (tolower2(c) - 'a');

Again, this only works if you are sure that 'a'..'f' is a consecutive set.
}
}

int htoi(char *c)
{
int i, k, prefix = 0;
size_t sum = 0;

if (c[0] == '0' && tolower2(c[1]) == 'x')
prefix = 1;

Are you sure that you have 2 valid characters in c? What happens when you
call htoi("7"), for example?

Any non-empty string will have valid values in c[0] and c[1]. Also, the
standard guarantees that '\0' != '0' :)
Try:

if (c[0] && c[0] == '0' && c[1] && (c[1] == 'x' || c[1] == 'X'))
prefix = 1;

It's admittably a bit less elegant but safer. By the way, you could work
on an algorithm without the variable prefix. It's really simple, think
about it a little. All you need is to skip the first 2 characters of c...
for (i = (prefix == 1)? 2:0 ; c ;i++ )
{
if (!isxdigit2(c) )
{
printf("Wrong hexa number\n");


"Hexa" as in "jinxed"? ;-)
return 0;
}
c = char_to_num(c);
}

for (k = (prefix == 1)? 2 : 0 ; k <= i-1 ; ++k )
{
sum += c[k] * power2(16, i-1-k);
}

return sum;
}

int main()
{
char c[] = "0xAB";
printf("%u", htoi(c));


Err, no sir. %u in printf() expects unsigned int, but you provide it with
the result of htoi(), which returns int. This is strictly speaking an
undefined behaviour, although admittably I have yet to see a platform
where it does not work. In any case, use %d instead or change your htoi()
to return unsigned int.
return 0;
}

It looks OK, though overly complicated. You could use already available
macros isxdigit and tolower (both defined in ctype.h), but frankly you
should not even need them. And you certainly should not need your
power2().

Think about it. What's 1986 in decimal?
In your algorithm, it's 1*10^3 + 9*10^2 + 8*10^1 + 1*10^0.
How about (((1)*10 + 9)*10 + 8)*10 + 6?

Now try converting it to a C program, for any (hexa)decimal number with
any number of digits.
//******************************************************************

when i change char c[] to char *c in main(),
it shows error, why ??

Undefined behaviour. Your htoi() tries to alter the string it is given in
situ. With char c[] in main, the string literal "0xAB" get copied to a
local array c, which is OK. But when you declare c as char *, you pass a
pointer to the string literal to htoi()... oops!


Waah... I missed that one, didn't I? Tricksy little hobbitses...

-nrk.
 
V

Vijay Kumar R Zanvar

How about this one?
#include <stdio.h>
#include <stdlib.h>
#include <ctype.h>
#include <math.h>
#include <string.h>

#define MAX_STR 80

void
reverse ( char * str )
{
char *beg, *end;

beg = str;
end = str + strlen ( str ) - 1;

while ( beg < end )
{
*beg ^= *end ^= *beg ^= *end;
beg++;
end--;
}
return;
}

int
to_num ( char dig )
{
if ( !isxdigit ( dig ) )
{
fprintf ( stderr, "Not a hexadecimal number" );
exit ( EXIT_FAILURE );
}

if ( isalpha ( dig ) )
return tolower ( dig ) - 'a' + 10;
else
return dig - '0';
}


int
main ( void )
{
char str[MAX_STR];
long dec = 0, exp = 0;
char *ptr;

printf ( "Enter a hexadecimal number: " ); if ( !fgets ( str, MAX_STR,
stdin ) ) return EXIT_FAILURE; /* discard "0x" or "0X" */
if ( str[0] == '0' )
if ( tolower ( str[1] ) == 'x' )
memmove ( str, str+2, 3 );
puts ( str );

/* use function reverse(s) of exercise 1-19.c */
reverse ( str );
ptr = str;

while ( *ptr )
{
dec = dec + to_num ( *ptr ) * pow ( 16, exp );
exp++;
ptr++;
}

printf ( "Decimal: %ld\n", dec );
return EXIT_SUCCESS;
}
 
P

Peter Pichler

nrk said:
Peter said:
Herrcho said:
int htoi(char *c)
{
int i, k, prefix = 0;
size_t sum = 0;

if (c[0] == '0' && tolower2(c[1]) == 'x')
prefix = 1;

Are you sure that you have 2 valid characters in c? What happens when you
call htoi("7"), for example?

Any non-empty string will have valid values in c[0] and c[1]. Also, the
standard guarantees that '\0' != '0' :)

Err... yes, but... mumbleohbuggerhumblegrumble... OK, I concede ;-)
 
N

nrk

Vijay said:
How about this one?

Did you even care to test it once? Apart from being needlessly complicated,
reliant on a contiguous ascending alphabet character set, unnecessarily
using real arithmetic, causing gratuituous undefined behavior, it is also
hopelessly broken because fgets may leave '\n' in your buffer.
#include <stdio.h>
#include <stdlib.h>
#include <ctype.h>
#include <math.h>
#include <string.h>

#define MAX_STR 80

void
reverse ( char * str )
{
char *beg, *end;

beg = str;
end = str + strlen ( str ) - 1;

Disaster waiting to happen when str == "".
while ( beg < end )
{
*beg ^= *end ^= *beg ^= *end;
beg++;
end--;

Needlessly cute. Try to use the language correctly before being "leet".
Whatever in the world is wrong with?:
*beg++ = *end--;
Do you object to it because it is more readable and demonstrably correct in
this context? Do you object to it because it is likely more efficient?
}
return;
}

int
to_num ( char dig )
{
if ( !isxdigit ( dig ) )
{
fprintf ( stderr, "Not a hexadecimal number" );

Missing '\n'.
exit ( EXIT_FAILURE );
}

if ( isalpha ( dig ) )
return tolower ( dig ) - 'a' + 10;

What if 'a' > 'b'? What if 'b' == 'a' + 3?
else
return dig - '0';
}


int
main ( void )
{
char str[MAX_STR];
long dec = 0, exp = 0;
char *ptr;

printf ( "Enter a hexadecimal number: " ); if ( !fgets ( str,
MAX_STR,
stdin ) ) return EXIT_FAILURE; /* discard "0x" or "0X" */
if ( str[0] == '0' )
if ( tolower ( str[1] ) == 'x' )
memmove ( str, str+2, 3 );

Oh Joy!! Even more undefined behavior. Who told you that str+3 and str+4
will contain something valid? If str == "0x" or str == "0x\n", you end up
touching uninitialized memory.
puts ( str );

/* use function reverse(s) of exercise 1-19.c */
reverse ( str );
ptr = str;

while ( *ptr )
{
dec = dec + to_num ( *ptr ) * pow ( 16, exp );
exp++;
ptr++;
}

printf ( "Decimal: %ld\n", dec );
return EXIT_SUCCESS;
}

Try to write unoptimised, "unleet", correct code before getting fancy. Try
to atleast test your code once before posting it [this one doesn't work for
one single valid case, except on stdin redirection and a non-standard file
with no ending newline that contains a valid hex number].

-nrk.
 
N

nrk

nrk said:
Did you even care to test it once? Apart from being needlessly
complicated, reliant on a contiguous ascending alphabet character set,
unnecessarily using real arithmetic, causing gratuituous undefined
behavior, it is also hopelessly broken because fgets may leave '\n' in
your buffer.


Disaster waiting to happen when str == "".


Needlessly cute. Try to use the language correctly before being "leet".
Whatever in the world is wrong with?:
*beg++ = *end--;
Do you object to it because it is more readable and demonstrably correct
in this context? Do you object to it because it is likely more efficient?

Got carried away there. You should object because it is demonstrably
incorrect :) Obviously you need to swap the two values with a temporary
intermediary:

char temp = *beg;
*beg++ = *end;
*end-- = temp;

Taking a dose of my own medicine, I should've tested the darn thing before
posting.

Also, that memmove breaks your program for most valid inputs starting with
0x.

-nrk.
 
J

Joe Wright

nrk said:
Got carried away there. You should object because it is demonstrably
incorrect :) Obviously you need to swap the two values with a temporary
intermediary:

char temp = *beg;
*beg++ = *end;
*end-- = temp;

Taking a dose of my own medicine, I should've tested the darn thing before
posting.

Also, that memmove breaks your program for most valid inputs starting with
0x.

-nrk.

Try this..

/*
Program: htol.c
Author: Joe Wright <[email protected]>

Convert a hexadecimal char to its numeric equivalent.
Accommodate ASCII and EBCDIC. Maybe others.
*/

#include <stdio.h>

typedef unsigned char uchar;
typedef unsigned long ulong;

/*
We will examine a printing hex character and determine its bin value.
Of course the numerics will translate directly. The alphas are a
special case as both 'a' and 'A' will evaluate to 10.
Input must be int within the ranges '0'..'9', 'A'..'F' and 'a'..'f'.

Some assumptions:

That '0',,'9', 'A'..'F' and 'a'..'f' are contiguous in the set.
'0'..'9' is guaranteed but the others are not. Oh well, they are
contiguous in ASCII and EBCDIC.

*/

/* Returns a value 0..15 for hex digits or -1 on failure. */

int h2b(int h) {
int i, b = -1; /* Error code */
if ((i = h - '0') >= 0 && i < 10)
b = i;
else if ((i = h - 'a') >= 0 && i < 6)
b = i + 10;
else if ((i = h - 'A') >= 0 && i < 6)
b = i + 10;
return b;
}

ulong h2l(char *h) {
ulong l = 0;
int c;
while ((c = *h++) && (c == '0' || c == 'x' || c == 'X'))
;
if (c)
do {
l = l * 16 + h2b(c);
} while((c = *h++));
return l;
}

int main(int argc, char *argv[]) {
ulong ans = 0;
if (argc > 1)
ans = h2l(argv[1]);
printf("\t%lu\n", ans);
return 0;
}
 
C

CBFalconer

Joe said:
.... snip ...

/*
We will examine a printing hex character and determine its bin value.
Of course the numerics will translate directly. The alphas are a
special case as both 'a' and 'A' will evaluate to 10.
Input must be int within the ranges '0'..'9', 'A'..'F' and 'a'..'f'.

Some assumptions:

That '0',,'9', 'A'..'F' and 'a'..'f' are contiguous in the set.
'0'..'9' is guaranteed but the others are not. Oh well, they are
contiguous in ASCII and EBCDIC.
*/

/* Returns a value 0..15 for hex digits or -1 on failure. */
int h2b(int h) {

Why make any assumptions?

/* also useful for reverse conversions */
static char[] hexchars = "0123456789abcdefABCDEF";

/* Returns a value 0..15 for hex digits or -1 on failure. */
int h2b(int h)
{
char * s;

if (NULL == (s = strchr(hexchars, h))) h = -1;
else {
h = s - hexchars;
if (h > 15) h = h - 6;
}
if (h > 15) h = -1; /* Exercise - why this */
return h;
}
 
C

Chris Torek

Why make any assumptions?

/* also useful for reverse conversions */
static char[] hexchars = "0123456789abcdefABCDEF";

You need to test these things before posting -- that should
be "static char hexchars[]". :)

More seriously, it might be worth making it a readonly array,
of type "const char".

[snippage]
if (h > 15) h = -1; /* Exercise - why this */

If you want to get rid of this, there is always memchr().
 
P

pete

Chris said:
Why make any assumptions?

/* also useful for reverse conversions */
static char[] hexchars = "0123456789abcdefABCDEF";

You need to test these things before posting -- that should
be "static char hexchars[]". :)

More seriously, it might be worth making it a readonly array,
of type "const char".

[snippage]
if (h > 15) h = -1; /* Exercise - why this */

If you want to get rid of this, there is always memchr().

or

s = (h != '\0' ? strchr(hexchars, h) : NULL);
 
J

Joe Wright

CBFalconer said:
Joe said:
... snip ...

/*
We will examine a printing hex character and determine its bin value.
Of course the numerics will translate directly. The alphas are a
special case as both 'a' and 'A' will evaluate to 10.
Input must be int within the ranges '0'..'9', 'A'..'F' and 'a'..'f'.

Some assumptions:

That '0',,'9', 'A'..'F' and 'a'..'f' are contiguous in the set.
'0'..'9' is guaranteed but the others are not. Oh well, they are
contiguous in ASCII and EBCDIC.
*/

/* Returns a value 0..15 for hex digits or -1 on failure. */
int h2b(int h) {

Why make any assumptions?

/* also useful for reverse conversions */
static char[] hexchars = "0123456789abcdefABCDEF";

/* Returns a value 0..15 for hex digits or -1 on failure. */
int h2b(int h)
{
char * s;

if (NULL == (s = strchr(hexchars, h))) h = -1;
else {
h = s - hexchars;
if (h > 15) h = h - 6;
}
if (h > 15) h = -1; /* Exercise - why this */
return h;
}
The string should be ..

static char hexchars[] = "0123456789abcdefABCDEF";

Exercise? There is a NUL at hexchars[22]. If h started off 0 strchr()
will find the NUL and 'h = s - hexchars' yields 22. 22 - 6 yields 16, an
error.

How'd I do coach? You got a job for me? In the Southwest if possible.
:)
 
C

CBFalconer

Joe said:
CBFalconer said:
Joe said:
... snip ...

/*
We will examine a printing hex character and determine its bin value.
Of course the numerics will translate directly. The alphas are a
special case as both 'a' and 'A' will evaluate to 10.
Input must be int within the ranges '0'..'9', 'A'..'F' and 'a'..'f'.

Some assumptions:

That '0',,'9', 'A'..'F' and 'a'..'f' are contiguous in the set.
'0'..'9' is guaranteed but the others are not. Oh well, they are
contiguous in ASCII and EBCDIC.
*/

/* Returns a value 0..15 for hex digits or -1 on failure. */
int h2b(int h) {

Why make any assumptions?

/* also useful for reverse conversions */
static char[] hexchars = "0123456789abcdefABCDEF";

/* Returns a value 0..15 for hex digits or -1 on failure. */
int h2b(int h)
{
char * s;

if (NULL == (s = strchr(hexchars, h))) h = -1;
else {
h = s - hexchars;
if (h > 15) h = h - 6;
}
if (h > 15) h = -1; /* Exercise - why this */
return h;
}
The string should be ..

static char hexchars[] = "0123456789abcdefABCDEF";

Exercise? There is a NUL at hexchars[22]. If h started off 0
strchr() will find the NUL and 'h = s - hexchars' yields 22.
22 - 6 yields 16, an error.

How'd I do coach? You got a job for me? In the Southwest if
possible. :)

You did better than I did. No. How about one for me, in the
Northeast. :-[

At any rate my point was: Don't make unnecessary assumptions, even
though they may be convenient.
 
I

Irrwahn Grausewitz

CBFalconer said:
Joe said:
CBFalconer wrote:
/* also useful for reverse conversions */
static char[] hexchars = "0123456789abcdefABCDEF";

/* Returns a value 0..15 for hex digits or -1 on failure. */
int h2b(int h)
{
char * s;

if (NULL == (s = strchr(hexchars, h))) h = -1;
else {
h = s - hexchars;
if (h > 15) h = h - 6;
}
if (h > 15) h = -1; /* Exercise - why this */
return h;
}
Exercise? There is a NUL at hexchars[22]. If h started off 0
strchr() will find the NUL and 'h = s - hexchars' yields 22.
22 - 6 yields 16, an error.
<snip>

My 2ct:

if ( h && (s = strchr( hexchars, h )) )
/*...*/

is a well-known idiom for retrieving the position of a non-null
character in a test string with strchr. Alternatively memchr
could be used, as suggested by Chris Torek elsethread.


Regards
 
H

Herrcho

Hi nrk~ Thanks for your comments. really good for my learning C.

i corrected my code.. Could you check it out ?

/*************************************************************************
#include <stdio.h>
#include <string.h>

int lowercase(char c)
{
static const char *uppercase = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
static const char *lowercase = "abcdefghijklmnopqrstuvwxyz";

char *cptr;
cptr = strchr(uppercase, c);
if ( cptr ) return lowercase[cptr - uppercase];
return c;
}

int char_to_num(char c)
{
char *k = "abcdef";

if (c >= '0' && c <= '9')
return c - '0';
else
return strchr(k, lowercase(c)) - k + 10;
}

int isxdigit2(char c)
{
static const char *hexalpha = "abcdefABCDEF";

if ( (c >= '0' && c <= '9') || (c && strchr(hexalpha, c)) )
return 1;
else
return 0;
}

unsigned htoi(char *c)
{
int sum = 0;

if ( c[0] == '0' && (c[1] == 'x' || c[1] == 'X') )
c += 2;
while (*c && isxdigit2(*c) )
{
sum = (sum * 16) + char_to_num(*c);
++c;
}
if ( *c ) {
fprintf(stdout, "Invalid hex digit %c in string\n", *c);
sum = 0;
}
return sum;
}

int main()
{
char *c = "0Xca";
printf("%u", htoi(c));

return 0;
}
/*****************************************************************

it runs well , but i have a couple of questions..

what's the difference when i change 'stderr' to 'stdout' in the below.

fprintf(stderr, "Invalid hex digit %c in string\n", *c);

it doesn't make any difference in my machine.

and in isxdigit2 function, when i remove 'static' keyword ,

from 'static const char *hexalpha = "abcdefABCDEF"; '
to ' const char *hexalpha = "abcdefABCDEF"; '

what's the difference ? i know what static means in C, but

i don't see why 'static' needs in isxdigit2 function.
 
C

CBFalconer

Herrcho said:
.... snip ...

what's the difference when i change 'stderr' to 'stdout' in the below.

fprintf(stderr, "Invalid hex digit %c in string\n", *c);

stderr and stdout are different files, but usually (not always)
connected to the 'terminal' by default. You can control the
destination for stdout in most OSs with redirection. If that is
done using stderr allows the error messages to remain visible on
the terminal.
and in isxdigit2 function, when i remove 'static' keyword ,

from 'static const char *hexalpha = "abcdefABCDEF"; '
to ' const char *hexalpha = "abcdefABCDEF"; '

what's the difference ? i know what static means in C, but
i don't see why 'static' needs in isxdigit2 function.

The first line loads hexalpha once at program startup. The second
loads it every time the function isxdigit2 is called, and
generates extra code to do so.
 
I

Irrwahn Grausewitz

i corrected my code.. Could you check it out ?

Now that you've called for it ... ;-)
/*************************************************************************
#include <stdio.h>
#include <string.h>

int lowercase(char c)
To be consistent with the standard library functions, I suggest
to use int for character arguments.
{
static const char *uppercase = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
static const char *lowercase = "abcdefghijklmnopqrstuvwxyz";
Nitpick:
static const char const *...
;-)
char *cptr;
Nit:
const char *cptr;
cptr = strchr(uppercase, c);
if ( cptr ) return lowercase[cptr - uppercase];
return c;
}
int char_to_num(char c) See above.
{
char *k = "abcdef";
Nit:
static const char const *k = "abcdef"
if (c >= '0' && c <= '9')
return c - '0';
else
return strchr(k, lowercase(c)) - k + 10;
}

And you better make sure to exclusively feed hexdigit characters
to the char_to_num function (in the call in main below you do, of
course).
int isxdigit2(char c)
See above, plus: identifiers beginning with 'is' followed by a
lowercase letter are reserved for future library extensions;
is_xdigit2 would be OK, though.
{
static const char *hexalpha = "abcdefABCDEF";
Nit: static const char const *...
if ( (c >= '0' && c <= '9') || (c && strchr(hexalpha, c)) )
return 1;
else
return 0;
}
unsigned htoi(char *c) unsigned htoi( const char *c )
{
int sum = 0;
Since you are going to return the value of sum, let it match
the return type:
unsigned int sum = 0;

I would use unsigned long (or maybe even size_t) as return type,
anyway.
if ( c[0] == '0' && (c[1] == 'x' || c[1] == 'X') )
c += 2;
while (*c && isxdigit2(*c) )
{
sum = (sum * 16) + char_to_num(*c);
++c;
}
if ( *c ) {
fprintf(stdout, "Invalid hex digit %c in string\n", *c);
sum = 0;
I'm not sure if altering the conversion result in this case is
the most sensible thing to do.
}
return sum;
}

int main()
{
char *c = "0Xca";
const char *c = ...
printf("%u", htoi(c));

return 0;
}
/*****************************************************************

it runs well ,
I assume you didn't compile with maximum warning level... ;-)
but i have a couple of questions..
what's the difference when i change 'stderr' to 'stdout' in the below.

fprintf(stderr, "Invalid hex digit %c in string\n", *c);

it doesn't make any difference in my machine.

The stdout and stderr streams may be associated with different
files/devices.

<OT>
Imagine your program being invoked from a unixish command shell
like this:

foo 2>foo_stderr_log

</OT>

There may also be a difference in buffering, see C99 7.19.3:

7 [...] As initially opened, the standard error stream is
not fully buffered; the standard input and standard output
streams are fully buffered if and only if the stream can
be determined not to refer to an interactive device.
and in isxdigit2 function, when i remove 'static' keyword ,

from 'static const char *hexalpha = "abcdefABCDEF"; '
to ' const char *hexalpha = "abcdefABCDEF"; '

what's the difference ? i know what static means in C, but
i don't see why 'static' needs in isxdigit2 function.

The static storage-class specifier really isn't needed in this
case, but it's sensible to use it: since the pointer isn't
changed throughout program execution, one single instance of
it is sufficient for the code at hand. Thus it's logical to
declare it static at function scope and let it being initialized
during program startup, rather than creating and destroying an
automatic instance each and every time the function is called or
returns, respectively. As mentioned above, the most pedantic
declaration is:

static const char const *hexalpha = "....";

You could also drop the pointer variable completely and put
the string literal in the call to strchr, but IMHO that would
just add to obfuscation and wouldn't have any real advantages.

HTH
Regards
 
N

nrk

Herrcho said:
Hi nrk~ Thanks for your comments. really good for my learning C.

i corrected my code.. Could you check it out ?

/*************************************************************************
#include <stdio.h>
#include <string.h>

int lowercase(char c)
{
static const char *uppercase = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
static const char *lowercase = "abcdefghijklmnopqrstuvwxyz";

char *cptr;
cptr = strchr(uppercase, c);
if ( cptr ) return lowercase[cptr - uppercase];
return c;
}

int char_to_num(char c)
{
char *k = "abcdef";

static const char *hexalpha = "abcdef";

See Irrwahn and CBF's answers for why static might be a good idea. Also,
try to give meaningful names for long-lived variables such as this one.
Calling it "k" is not useful at all. Calling it hexalpha ostensibly
provides some idea of what you're trying to achieve.
if (c >= '0' && c <= '9')
return c - '0';
else
return strchr(k, lowercase(c)) - k + 10;
}

See Irrwahn's point about this function. I hit the post button in my
original reply and then thought of the issue Irrwahn brought up. It is a
good point. While in your current code, char_to_num is guaranteed to
receive only a valid lowercase hex digit, on your next cut-n-paste of this
"working" char_to_num, you may get a rather nasty surprise if that
pre-condition is not satisfied. So, here's a char_to_num that fails
gracefully:

int char_to_num(char c) {
static const char *lhexalpha = "abcdef";
char *cptr;

if ( c >= '0' && c <= '9' ) return c - '0';
else if ( c && (cptr = strchr(lhexalpha, c)) )
return 10 + cptr - hexalpha;
return -1; /* Negative return indicates failure */
}

Keep in mind that this char_to_num doesn't handle uppercase hex digits.
That's fine in the current context, but I would probably add a comment to
the function to avoid a surprise when I cut-n-paste this code somewhere
else later on.
int isxdigit2(char c)
{
static const char *hexalpha = "abcdefABCDEF";

if ( (c >= '0' && c <= '9') || (c && strchr(hexalpha, c)) )

I didn't elaborate on the (c && strchr(hexalpha, c)) part. But if you
didn't give it much thought, see CBF's post upthread with his little
"exercise" to see why this is important.
return 1;
else
return 0;
}

unsigned htoi(char *c)
{
int sum = 0;

If you're returning unsigned, why not make sum match that return?
unsigned sum = 0;
if ( c[0] == '0' && (c[1] == 'x' || c[1] == 'X') )
c += 2;
while (*c && isxdigit2(*c) )
{
sum = (sum * 16) + char_to_num(*c);
++c;
}
if ( *c ) {
fprintf(stdout, "Invalid hex digit %c in string\n", *c);

Error messages are better off in the standard error stream. I don't have my
K&R1 with me (it's thousands of miles away right now, and I definitely do
miss it. If it weren't out of my student budget, I would definitely buy
another copy :-( ), but I am sure it explains standard input (stdin),
standard output (stdout) and standard error (stderr), the three streams
that are available to you at the start of your program.
sum = 0;
}
return sum;
}

int main()
{
char *c = "0Xca";
printf("%u", htoi(c));

return 0;
}
/*****************************************************************

it runs well , but i have a couple of questions..

what's the difference when i change 'stderr' to 'stdout' in the below.

fprintf(stderr, "Invalid hex digit %c in string\n", *c);

it doesn't make any difference in my machine.

See Irrwahn's answer. On typical systems, both stdout and stderr are tied
to your terminal by default. However, if you're on a Unixish system try
out his suggestion and view the contents of foo_stderr_log (or the file you
redirect stderr to) to see the difference between my version and yours.
and in isxdigit2 function, when i remove 'static' keyword ,

from 'static const char *hexalpha = "abcdefABCDEF"; '
to ' const char *hexalpha = "abcdefABCDEF"; '

what's the difference ? i know what static means in C, but

i don't see why 'static' needs in isxdigit2 function.

It is not needed, but is probably a good idea (excellent answers from others
tell you why, and there's nothing I can add).

-nrk.
 
N

nrk

Irrwahn said:
Now that you've called for it ... ;-)

To be consistent with the standard library functions, I suggest
to use int for character arguments.
Nitpick:
static const char const *...

Nitpick, ITYM:
static const char * const ...

Here, and in other places as well :)

-nrk.
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

No members online now.

Forum statistics

Threads
473,744
Messages
2,569,482
Members
44,901
Latest member
Noble71S45

Latest Threads

Top