strupr Postmortem Analysis

[humanmutantman]

I found a string to upper function that looks like this:

char *strupr(char *string)
{
char *s;
if (string)
{
for (s = string; *s; ++s)
*s = toupper(*s);
}
return string;
}

It works and all is ok, but I thought I could trim it down to the
following that doesn't work:

char *strupr(char *string)
{
// char *s;
if (string)
{
// for (s = string; *s; ++s)
for (string; *string; ++string)
// *s = toupper(*s);
*string = toupper(*string);
}
return string;
}

Can someone explain why the bottom function doesn't work? Thanks!

 

[Chris Smith]

I found a string to upper function that looks like this:

char *strupr(char *string)
{
char *s;
if (string)
{
for (s = string; *s; ++s)
*s = toupper(*s);
}
return string;
}

It works and all is ok, but I thought I could trim it down to the
following that doesn't work:

char *strupr(char *string)
{
// char *s;
if (string)
{
// for (s = string; *s; ++s)
for (string; *string; ++string)
// *s = toupper(*s);
*string = toupper(*string);
}
return string;
}

Can someone explain why the bottom function doesn't work? Thanks!

Yes. The original function kept a copy of the beginning of the string
('string'), and returned it. In the second function, you are returning
the modified pointer, which now points to the terminating NUL character.

 

[humanmutantman]

I found a string to upper function that looks like this:

char *strupr(char *string)
{
char *s;
if (string)
{
for (s = string; *s; ++s)
*s = toupper(*s);
}
return string;
}

It works and all is ok, but I thought I could trim it down to the
following that doesn't work:

line 1> char *strupr(char *string)
line 2> {
line 3> // char *s;
line 4> if (string)
line 5> {
line 6> // for (s = string; *s; ++s)
line 7> for (string; *string; ++string)
line 8> // *s = toupper(*s);
line 9> *string = toupper(*string);
line 10> }
line 11> return string;
line 12> }

Can someone explain why the bottom function doesn't work? Thanks!

I see part of the problem: line 9, "*string = toupper(*string);". I'm
stepping over the contents of the pointer "string", yes? As the
modified code is written, every instance through the for loop, *string
is given some new value which then breaks, yes?

If someone could step me through the loop that might clear things up
for me. I'm new to pointers... Thanks.

 

[Robert Gamble]

I found a string to upper function that looks like this:

char *strupr(char *string)

Identifiers beginning with "str" followed by a lowercase letter are
reserved for future string functions.

{
char *s;
if (string)
{
for (s = string; *s; ++s)
*s = toupper(*s);

That should really be "*s = toupper((unsigned char)*s);", char can be
signed and passing a value that does not fit into an unsigned char and
is not EOF is undefined.

}
return string;
}

It works and all is ok, but I thought I could trim it down to the
following that doesn't work:

char *strupr(char *string)
{
// char *s;
if (string)
{
// for (s = string; *s; ++s)
for (string; *string; ++string)
// *s = toupper(*s);
*string = toupper(*string);
}
return string;
}

Can someone explain why the bottom function doesn't work? Thanks!

What do you mean by "doesn't work"? What did it do differently than
what you expected? Where is the actual code that you are using that
demonstrates the problem?

The second function is not functionally identical to the first one; the
first function returns a pointer to the beginning of the string
provided, the second function returns a pointer to the end of the
string. If you are doing something like this:

char s[] = "test";
printf("%s\n", strupr(s));

the result probably won't be what you expect whereas this:

char s[] = "test";
strupr(s);
printf("%s\n", s);

will work as expected.

It should be noted that since the function operates on the string
provided that something like this would result in undefined behavior:

strupr("test");

The first function can be reduced to something like this:

char *strupr(char *string)
{
char *s = string;
if (s)
for (s; *s; ++s)
*s = toupper(*s);
return string;
}

(Some people would object to the lack of brackets, add them as you see
fit.)

or even:

char *strupr(char *string)
{
char *s = string;
if (s)
do { *s = toupper(*s); } while (*s++);
return string;
}

Robert Gamble

 

[Ian Collins]

Identifiers beginning with "str" followed by a lowercase letter are
reserved for future string functions.




That should really be "*s = toupper((unsigned char)*s);", char can be
signed and passing a value that does not fit into an unsigned char and
is not EOF is undefined.

Is it? I thought toupper and tolower return their argument unchanged if
its value is invalid.

 

[Frederick Gotham]

posted:

I found a string to upper function that looks like this:

The following is far simpler:


#include <ctype.h>

void StringUp( char *p )
{
while( *p = toupper(*p) ) ++p;
}

 

[Robert Gamble]

Is it? I thought toupper and tolower return their argument unchanged if
its value is invalid.

7.4p1 (ctype.h):
"The header <ctype.h> declares several functions useful for classifying
and mapping
characters. In all cases the argument is an int, the value of which
shall be
representable as an unsigned char or shall equal the value of the macro
EOF. If the
argument has any other value, the behavior is undefined."

7.4.2.1p3 (tolower):
If the argument is a character for which isupper is true and there are
one or more
corresponding characters, as specified by the current locale, for which
islower is true,
the tolower function returns one of the corresponding characters
(always the same one
for any giv en locale); otherwise, the argument is returned unchanged.

Robert Gamble

 

[Robert Gamble]

posted:



The following is far simpler:


#include <ctype.h>

void StringUp( char *p )
{
while( *p = toupper(*p) ) ++p;

while(*p = toupper((unsigned char)*p) ) ++p;

}

Simpler, yes, but it doesn't do the same thing as the function the OP
presented, namely returning a pointer to the beginning of the string
provided.

Robert Gamble

 

[Ian Collins]

7.4p1 (ctype.h):
"The header <ctype.h> declares several functions useful for classifying
and mapping
characters. In all cases the argument is an int, the value of which
shall be
representable as an unsigned char or shall equal the value of the macro
EOF. If the
argument has any other value, the behavior is undefined."

7.4.2.1p3 (tolower):
If the argument is a character for which isupper is true and there are
one or more
corresponding characters, as specified by the current locale, for which
islower is true,
the tolower function returns one of the corresponding characters
(always the same one
for any giv en locale); otherwise, the argument is returned unchanged.

Can't argue with that.

 

[Frederick Gotham]

Robert Gamble posted:

while(*p = toupper((unsigned char)*p) ) ++p;

This would imply that the integer at address "p" might be negative.

If the integer is negative, then do we not have a error? Forcing the
negative number to be positive won't solve the problem.

If anything, would the following not be better:

#include <assert.h>
#include <ctype.h>

void StringUp( char *p )
{
do assert( *p >= 0 );
while( *p = toupper( *p ), *p++ );
}

#include <stdio.h>

int main(void)
{
char array[] = "The man walked by the 3rd door and turned right.";

StringUp(array);

puts(array);
}

 

[Chris Torek]

Robert Gamble posted:

This would imply that the integer at address "p" might be negative.

As indeed it might.

If the integer is negative, then do we not have a error?

Maybe, but probably not:

/* signed */ char buf[] = "sláinte";
/* signed */ char *p = buf;

while ((*p = toupper((unsigned char)*p)) != '\0')
p++;
puts(buf);

Without the "unsigned", on my systems where plain "char" is signed,
the word is mangled; with it, on those same systems, it works right
(producing SLÁINTE).

 

[Robert Gamble]

Robert Gamble posted:




This would imply that the integer at address "p" might be negative.

Right, since char may be signed it is certainly possible for the values
of an array of char to be negative.

If the integer is negative, then do we not have a error?

What would be erroneous about it?

Forcing the
negative number to be positive won't solve the problem.

There isn't a problem except the fact that you can't pass a negative
value (except EOF) to the toupper function and casting the value to
unsigned char certainly does address that.

If anything, would the following not be better:

#include <assert.h>
#include <ctype.h>

void StringUp( char *p )
{
do assert( *p >= 0 );

Why do you think this *p can't be negative?

while( *p = toupper( *p ), *p++ );

Yuck, 0 points for style.

}

#include <stdio.h>

int main(void)
{
char array[] = "The man walked by the 3rd door and turned right.";

StringUp(array);

puts(array);
}

It is possible for char to be signed. It is also possible for, in some
locale, there to be letters that have negative values. The only way to
perform portable case conversions on these characters would be to use
the toupper/tolower functions and the only way to do that without
invoking undefined behavior is to cast them to unsigned char. The
implementation will be expecting to receive such characters so casted.
I don't understand where you think the problem lies here.

Robert Gamble

 

[humanmutantman]

I found a string to upper function that looks like this:

Identifiers beginning with "str" followed by a lowercase letter are
reserved for future string functions.


That should really be "*s = toupper((unsigned char)*s);", char can be
signed and passing a value that does not fit into an unsigned char and
is not EOF is undefined.


What do you mean by "doesn't work"? What did it do differently than
what you expected? Where is the actual code that you are using that
demonstrates the problem?

Good point . It did work, but sent "nothing" back to the screen... I
was expecting to see the capitalized version of the text that was sent
in.

The second function is not functionally identical to the first one; the
first function returns a pointer to the beginning of the string
provided, the second function returns a pointer to the end of the
string. If you are doing something like this:

char s[] = "test";
printf("%s\n", strupr(s));

the result probably won't be what you expect whereas this:

char s[] = "test";
strupr(s);
printf("%s\n", s);

will work as expected.

It should be noted that since the function operates on the string
provided that something like this would result in undefined behavior:

strupr("test");

The first function can be reduced to something like this:

char *strupr(char *string)
{
char *s = string;
if (s)
for (s; *s; ++s)
*s = toupper(*s);
return string;
}

(Some people would object to the lack of brackets, add them as you see
fit.)

or even:

char *strupr(char *string)
{
char *s = string;
if (s)
do { *s = toupper(*s); } while (*s++);
return string;
}

Robert Gamble

Most cool. Thanks for the help!

 

[humanmutantman]

I found a string to upper function that looks like this:

The following is far simpler:


#include <ctype.h>

void StringUp( char *p )
{
while( *p = toupper(*p) ) ++p;
}

I changed your code to this and got "null" back...

1 #include <ctype.h>
2 #include <stdio.h>
3
4 //void StringUp( char *p )
5 char StringUp( char *p )
6 {
7 while( *p = toupper(*p) ) ++p;
8 return *p;
9 }
10
11 int main()
12 {
13 char str[20] = {"this is a test"};
14 printf("--> %s\n",StringUp(str));
15 return 0;
16 }

Is there anyway to salvage what I did to get the all caps string
returned from the function?

 

[humanmutantman]

while(*p = toupper((unsigned char)*p) ) ++p;

This would imply that the integer at address "p" might be negative.

If the integer is negative, then do we not have a error? Forcing the
negative number to be positive won't solve the problem.

If anything, would the following not be better:

#include <assert.h>
#include <ctype.h>

void StringUp( char *p )
{
do assert( *p >= 0 );
while( *p = toupper( *p ), *p++ );
}

#include <stdio.h>

int main(void)
{
char array[] = "The man walked by the 3rd door and turned right.";

StringUp(array);

puts(array);
}

That looks interesting. I tried to convert your code to a function that
would return a value and came up with this that seg faults:

1 #include <assert.h>
2 #include <stdio.h>
3
4 char StringUp( char *p )
5 {
6 do assert( *p >= 0 );
7 while( *p = toupper( *p ), *p++ );
8 return *p;
9 }
10
11 int main(void)
12 {
13 char array[] = "The man walked by the 3rd door and turned
right.";
14 printf("--> %s\n",StringUp(array));
15 }

I would think that line 8 should be "return p", but I get:
test.c:8: warning: return makes integer from pointer without a cast

Once I get the code right to return a value I want to stick some
printf's in the function to trace what's going on. Right now I'm not
100%... Thanks!

 

[Richard Heathfield]

(e-mail address removed) said:

That looks interesting. I tried to convert your code to a function that
would return a value and came up with this that seg faults:

1 #include <assert.h>
2 #include <stdio.h>
3
4 char StringUp( char *p )

You have defined StringUp to return a single char...

10
11 int main(void)
12 {
13 char array[] = "The man walked by the 3rd door and turned
right.";
14 printf("--> %s\n",StringUp(array));

....but here you treat it as if it returned a string.

15 }

I would think that line 8 should be "return p", but I get:
test.c:8: warning: return makes integer from pointer without a cast

If you make it "return p;" you'll be returning a pointer to the null
character that terminates the string, which may or may not be what you want
but printf won't display very much.

 

[Roland Csaszar]

I changed your code to this and got "null" back...

1 #include <ctype.h>
2 #include <stdio.h>
3
4 //void StringUp( char *p )
5 char StringUp( char *p )

If you want to return a string and not a single character, you should use
char *, so char *StringUp( char *p ) is what you propably wanted.

6 {
7 while( *p = toupper(*p) ) ++p;

This loop is executed until *p == 0 (the end of the string is reached)

8 return *p;

so *p == 0 at this point, and you are always returning 0 (not the character
'0', but 0, the character '\0').

9 }
10
11 int main()
12 {
13 char str[20] = {"this is a test"};
14 printf("--> %s\n",StringUp(str));
15 return 0;
16 }

Is there anyway to salvage what I did to get the all caps string
returned from the function?

Yes, again you must somehow get the pointer to the first character and
return this pointer.
Like so:
char *StringUp( char *p )
{
char *string_start = p;
while( *p = toupper(*p) ) ++p;
return string_start;
}

 

[Roland Csaszar]

1 #include <assert.h>
2 #include <stdio.h>
3
4 char StringUp( char *p )

----------^^^^^

Should be char *StringUp( char *p )

5 {
6 do assert( *p >= 0 );
7 while( *p = toupper( *p ), *p++ );
8 return *p;

You are returning the 0 ('\0') which is positioned after the last character
of the string.

9 }
10
11 int main(void)
12 {
13 char array[] = "The man walked by the 3rd door and turned
right.";
14 printf("--> %s\n",StringUp(array));
15 }

I would think that line 8 should be "return p", but I get:

Yes, it should be return p (well, not really, but you should return a
pointer to a char, not a char)

test.c:8: warning: return makes integer from pointer without a cast

And this errormessage tells you, that the type of the return value is wrong.
You are telling the compiler that you are returning a char (an integer) in
the function's definition, but return a pointer to a char (a pointer to an
integer).

 

[humanmutantman]

I changed your code to this and got "null" back...

If you want to return a string and not a single character, you should use
char *, so char *StringUp( char *p ) is what you propably wanted.

Ahhh, I see the reasoning now...

This loop is executed until *p == 0 (the end of the string is reached)

I didn't understand that contents of the "while" condition would be
executed when the condition is true. That's pretty fancy stuff.

so *p == 0 at this point, and you are always returning 0 (not the character
'0', but 0, the character '\0').

Would you not be returning a pointer to "cell zero" of the pointer "p"?
To me, that seems like what is going on, so that when line 14 prints
the string, the string starts at "cell zero" and continues printing
each "cell" until, but not including, the null character "\0". No?

9 }
10
11 int main()
12 {
13 char str[20] = {"this is a test"};
14 printf("--> %s\n",StringUp(str));
15 return 0;
16 }

Is there anyway to salvage what I did to get the all caps string
returned from the function?

Yes, again you must somehow get the pointer to the first character and
return this pointer.
Like so:
char *StringUp( char *p )
{
char *string_start = p;
while( *p = toupper(*p) ) ++p;
return string_start;
}

Wow. That's good stuff! C is becoming fun learn. Thanks a lot for
clearing that up!

 

[humanmutantman]

That looks interesting. I tried to convert your code to a function that
would return a value and came up with this that seg faults:

1 #include <assert.h>
2 #include <stdio.h>
3
4 char StringUp( char *p )

You have defined StringUp to return a single char...

10
11 int main(void)
12 {
13 char array[] = "The man walked by the 3rd door and turned
right.";
14 printf("--> %s\n",StringUp(array));

...but here you treat it as if it returned a string.

15 }

I would think that line 8 should be "return p", but I get:
test.c:8: warning: return makes integer from pointer without a cast

If you make it "return p;" you'll be returning a pointer to the null
character that terminates the string, which may or may not be what you want
but printf won't display very much.

1 #include <assert.h>
2 #include <stdio.h>
3
4 char *StringUp(char *p)
5 {
6 char *string_start=p;
7 while(*p=toupper(*p))++p;
8 return string_start;
9 }
10
11 int main(void)
12 {
13 char array[]="The man walked by the 3rd door and turned
right.";
14 printf("--> %s\n",StringUp(array));
15 }

Ok, let's now say the new code is in lines 1-15 above (thanks Roland!).

If in line 8 I have "return string_start;", I get my desired results.
Is what is being returned a pointer? I thought pointers had to be
prefaced with an "*". Yes?

Is there some way I can printf the memory addresses of the content of
*string_start?