C - Is this str

Author

Is this str_rev() ok?

ballpointpenthief

2006-06-27, 6:56 pm

I was messing around with an exercise I found.
This seems to work, except in debugging, some VERY odd stuff happens,
so I thought I'd chuck the code on here to see how it stands up.

I had to make char *y global in order to free it. Disgusting.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

char *rev_str(char *);
char *y;
int main(void)
{
char *stToRev = " Dontchajustwishthisstringwouldgetreverse
d";
printf("%s\n", rev_str(stToRev));
free(y);
return 0;
}

char *rev_str(char *x)
{
size_t arrLength = strlen(x)+1;
y = malloc(arrLength);
memcpy(y,x,arrLength);
y = &y[arrLength-1];
while (*--y = *x++);
*y++;
return y;
}

Richard Heathfield

2006-06-27, 6:56 pm

ballpointpenthief said:

> I was messing around with an exercise I found.
> This seems to work, except in debugging, some VERY odd stuff happens,
> so I thought I'd chuck the code on here to see how it stands up.
>
> I had to make char *y global in order to free it. Disgusting.

No, you could have done this:

char *ptr = rev_str(stToRev);
if(ptr != NULL)
{
printf("%s\n", ptr);
free(ptr);
}

No need for file scope.

<snip>
>
> char *rev_str(char *x)

Since you don't plan to change x, why not make it const char *?

> {
> size_t arrLength = strlen(x)+1;
> y = malloc(arrLength);
> memcpy(y,x,arrLength);

That's fine (except that you probably want to use a local for y, rather than
a file scope object), provided malloc succeeds. But if it fails, you really
really really don't want to be memcpying to the result.

So check for NULL first.

> y = &y[arrLength-1];

Don't mod that pointer. Use a temp. Note that, if arrLength is 1 (which it
will be if x is ""), you are at the beginning of the array...

> while (*--y = *x++);

....which means this bit will break.

--
Richard Heathfield
"Usenet is a strange place" - dmr 29/7/1999
http://www.cpax.org.uk
email: rjh at above domain (but drop the www, obviously)

ballpointpenthief

2006-06-27, 6:56 pm

Richard Heathfield wrote:
> No, you could have done this:

What this?

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

char *rev_str(char *);
int main(void)
{
char *stToRev = "A man, a plan, a canal; Panama.";
char *ptr = rev_str(stToRev);
if (ptr)
{
printf("%s\n", ptr);
free(ptr);
}
return 0;
}

char *rev_str(char *x)
{
char *tmp, *y;
size_t arrLength = strlen(x)+1;

y = malloc(arrLength);
if (y == NULL)
return NULL;
if (!memcpy(y,x,arrLength))
return NULL;
tmp = &y[arrLength-1];
while (*--tmp = *x++);
return y;
}

Is the debugger I'm using broken then? It's doing some ODD stuff.
There is also a complaint about the while (*--tmp = *x++); when showing
all warnings:

Wedit output window build: Tue Jun 27 19:40:31 2006
Warning c:\lcc\projects\str_rev.c: 29 Assignment within conditional
expression
Compilation + link time:0.1 sec, Return code: 0

Jack Klein

2006-06-27, 6:56 pm

On 27 Jun 2006 11:00:34 -0700, "ballpointpenthief"
<Matt.Smiglarski@gmail.com> wrote in comp.lang.c:

> I was messing around with an exercise I found.
> This seems to work, except in debugging, some VERY odd stuff happens,
> so I thought I'd chuck the code on here to see how it stands up.

Very odd stuff indeed.

> I had to make char *y global in order to free it. Disgusting.

No, you didn't. See the lines I added to your main().

I'm going to pretend you used a much smaller original string for
illustrative purposes, namely "123".

> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
>
> char *rev_str(char *);

Make the prototype:

char *rev_str(const char *);

....since you don't modify the input string and the function is safe
for string literals and constant strings.

> char *y;
> int main(void)
> {
> char *stToRev = " Dontchajustwishthisstringwouldgetreverse
d";

char *rev = rev_str(stTdRev);

> printf("%s\n", rev_str(stToRev));
> free(y);

Now replace the two lines above with:

printf("%s\n", rev);
free(rev);

> return 0;
> }
>
> char *rev_str(char *x)

Again:

char *rev_str(const char *x)

> {
> size_t arrLength = strlen(x)+1;

Good, adding the +1. For the string "123", arrLength is 4.

> y = malloc(arrLength);

Bad, not checking for a NULL return from malloc().

> memcpy(y,x,arrLength);
> y = &y[arrLength-1];

Both lines above invokes undefined behavior if malloc() was
unsuccessful and returned a null pointer. Also it is completely
unnecessary, what do you gain by copying the contents in the original
order if you are only going to write over them with the reverse order?
If all you wanted to do was '\0' terminate the new string, it would
surely be more efficient, and much clearer, to do:

y += arrLength - 1;
*y = '\0';

> while (*--y = *x++);

For the moment, let's assume your memcpy() code is still in place, so
as this loop starts, here is what the two memory areas and the two
pointers look like on each pass through the loop:

Before pass 1: orig = 1 2 3 \0 rev = 1 2 3 \0
x y

Before pass 2: orig = 1 2 3 \0 rev = 1 2 1 \0
x y

Before pass 3: orig = 1 2 3 \0 rev = 1 2 1 \0
x y

Before pass 3: orig = 1 2 3 \0 rev = 3 2 1 \0
x y

Before pass 4: orig = 1 2 3 \0 rev = 3 2 1 \0
x y

During pass 4, you first decrement 'y' to point to one byte before the
allocated memory, which is in itself undefined behavior. You can
always form a pointer to the object one past the end of valid memory,
whether it is dynamically allocated or not, but there is no guarantee
that you can form a pointer before the beginning of the block.

But even worse, you write, or attempt to write, a '\0' null character
from the initial string into this byte one before the allocated
memory. That's a serious error, and most likely the cause of your
"odd" behavior.

> *y++;
> return y;

It's too late now, because your program is already off into
never-never land, but you don't need to dereference the pointer just
to increment it, and you don't need to compute the value before the
return statement.

You have no use for this, because when you rewrite the algorithm
properly you will never point to before the beginning of the allocated
block and need to increment the pointer. But if you did need to
increment a pointer before returning its value, you could combine
these two lines into one, either:

return ++y;

....or:

return y + 1;

> }

--
Jack Klein
Home: http://JK-Technology.Com
FAQs for
comp.lang.c http://c-faq.com/
comp.lang.c++ http://www.parashift.com/c++-faq-lite/
alt.comp.lang.learn.c-c++
http://www.contrib.andrew.cmu.edu/~.../FAQ-acllc.html

Roberto Waltman

2006-06-27, 6:56 pm

On 27 Jun 2006 11:37:28 -0700, "ballpointpenthief"
>There is also a complaint about the while (*--tmp = *x++); when showing
>all warnings:
>
>Wedit output window build: Tue Jun 27 19:40:31 2006
>Warning c:\lcc\projects\str_rev.c: 29 Assignment within conditional
>expression

That is normal. It is a common error in C to use an assignment when a
comparison was intended. It doesn't mean it is wrong, it means it
could be wrong and you should double check.

That is, instead of this:

char c;
...
if ( c == 'X')
{ // do this only when c equals 'X'
}

Doing this:

if (c = 'X')
{ // assign 'X' to c and do this always.
}

Malcolm

2006-06-27, 6:56 pm

--
Buy my book 12 Common Atheist Arguments (refuted)
$1.25 download or $7.20 paper, available www.lulu.com/bgy1mm

"ballpointpenthief" <Matt.Smiglarski@gmail.com> wrote in message
news:1151433448.508239.61050@b68g2000cwa.googlegroups.com...
>
> Richard Heathfield wrote:
>
> What this?
>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
>
> char *rev_str(char *);
> int main(void)
> {
> char *stToRev = "A man, a plan, a canal; Panama.";
> char *ptr = rev_str(stToRev);
> if (ptr)
> {
> printf("%s\n", ptr);
> free(ptr);
> }
> return 0;
> }
>
> char *rev_str(char *x)
> {
> char *tmp, *y;
> size_t arrLength = strlen(x)+1;
>
> y = malloc(arrLength);
> if (y == NULL)
> return NULL;
> if (!memcpy(y,x,arrLength))
> return NULL;
> tmp = &y[arrLength-1];
> while (*--tmp = *x++);
> return y;
> }
>
> Is the debugger I'm using broken then? It's doing some ODD stuff.
> There is also a complaint about the while (*--tmp = *x++); when showing
> all warnings:
>
That's a lot better.

Your main problem is the line

while(*--tmp = *x++);

you will often see experienced programmers write C code like that. However
it is hard to read, and it is asking for a logical error.
In your case, you are putting the terminating null at the start rather than
the end of the string. You wnat to reverse all the characters of the string,
except the terminating null.
If you rewrite this line to take three or four lines, and follow the logic,
the function will probably work.

Frederick Gotham

2006-06-27, 6:56 pm

Malcolm posted:

> Your main problem is the line
>
> while(*--tmp = *x++);
>
> you will often see experienced programmers write C code like that.
> However it is hard to read, and it is asking for a logical error.

I'm not with you on this one -- I frequently write code just like it.
Ever read TC++PL by Bjarne? He demonstrates a way of copying a string:

while ( *dest++ = *source++ );

Sure it might baffle a novice... but it's a-okay to me!

Looking at the following line:

while(*--tmp = *x++);

It's obvious that there's four simple steps:

(1) Decrement "tmp".
(2) Store "*x" in "*tmp".
(3) Increment "x".
(4) Repeat until 0 is encountered.

--

Frederick Gotham

Eric Sosman

2006-06-27, 6:56 pm

Malcolm wrote On 06/27/06 14:52,:
> [Something that's a little hard to quote, because the entire
> message appeared after the "-- \n" line and thus became part
> of the signature. It's all well and good to avoid top-posting,
> but this is taking things too far!]

The crucial bits are here reconstituted:

>
> Your main problem is the line
>
> while(*--tmp = *x++);
>
> you will often see experienced programmers write C code like
> that. However it is hard to read, and it is asking for a
> logical error. In your case, you are putting the terminating
> null at the start rather than the end of the string. [...]

In fact, it puts the terminating null not *at* the
beginning of the string, but *before* the beginning of
the string. Or, rather, it tries to; trying to store
something before the start of the allocated area yields
Undefined Behavior, and there's no telling what happens.

The O.P. complains of "VERY odd stuff" when he tries
to run this code under a debugger, and I have a hunch this
off-by-one error is the cause of at least some of the oddity.

--
Eric.Sosman@sun.com

Malcolm

2006-06-27, 6:56 pm

"Frederick Gotham" <fgothamNO@SPAM.com> wrote
> Malcolm posted:
>
>
>
> I'm not with you on this one -- I frequently write code just like it.
> Ever read TC++PL by Bjarne? He demonstrates a way of copying a string:
>
> while ( *dest++ = *source++ );
>
> Sure it might baffle a novice... but it's a-okay to me!
>
You are a clever person.
Others are baffled.

--
Buy my book 12 Common Atheist Arguments (refuted)
$1.25 download or $7.20 paper, available www.lulu.com/bgy1mm

pete

2006-06-27, 6:56 pm

ballpointpenthief wrote:
>
> Richard Heathfield wrote:
>
> What this?
>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
>
> char *rev_str(char *);
> int main(void)
> {
> char *stToRev = "A man, a plan, a canal; Panama.";
> char *ptr = rev_str(stToRev);
> if (ptr)
> {
> printf("%s\n", ptr);
> free(ptr);
> }
> return 0;
> }
>
> char *rev_str(char *x)
> {
> char *tmp, *y;
> size_t arrLength = strlen(x)+1;
>
> y = malloc(arrLength);
> if (y == NULL)
> return NULL;
> if (!memcpy(y,x,arrLength))
> return NULL;
> tmp = &y[arrLength-1];
> while (*--tmp = *x++);
> return y;
> }

I tried not to change it too much:

/* BEGIN new.c */

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

char *rev_str(char *);

int main(void)
{
char *stToRev = "A man, a plan, a canal; Panama.";
char *ptr = rev_str(stToRev);

if (ptr != NULL) {
puts(ptr);
free(ptr);
} else {
puts("ptr == NULL");
}
return 0;
}

char *rev_str(char *x)
{
char *tmp_y, *y;
size_t arrLength = strlen(x);

y = malloc(arrLength + 1);
if (y != NULL) {
tmp_y = y + arrLength;
*tmp_y = '\0';
while (tmp_y-- > y) {
*tmp_y = *x++;
}
}
return y;
}

/* END new.c */

--
pete

Skarmander

2006-06-27, 6:56 pm

Frederick Gotham wrote:
> Malcolm posted:
>
>
>
> I'm not with you on this one -- I frequently write code just like it.

Then you're doing people a disservice.

> Ever read TC++PL by Bjarne? He demonstrates a way of copying a string:
>
> while ( *dest++ = *source++ );
>
> Sure it might baffle a novice... but it's a-okay to me!
>
Well, to each their own. I would probably use a call to strcpy(); much faster.

Unless I'm implementing strcpy(), of course. Then I'd probably use assembler.

In the unlikely case I have to implement strcpy() in standard C, I'd
probably use

while ((*dest++ = *source++) != '\0');

This is a few more keystrokes, but won't require everyone who compiles it to
figure out that 1) I in fact know what I'm doing and 2) what the
-Wno-warn-on-that-thing-thats-nearly-always-an-error option for their
compiler is.

> Looking at the following line:
>
> while(*--tmp = *x++);
>
>
> It's obvious that there's four simple steps:
>
> (1) Decrement "tmp".
> (2) Store "*x" in "*tmp".
> (3) Increment "x".
> (4) Repeat until 0 is encountered.
>

while (*x) {
*--tmp = *x++;
}

Two more lousy lines, and you have removed a warning ubiquitous to nearly
all compilers, clarified the loop logic, and corrected the error.

That is, I'm assuming you noticed that the one-liner loop didn't do quite
what the OP expected it would do.

DTRT.

S.

pete

2006-06-27, 6:56 pm

Skarmander wrote:
>
> Frederick Gotham wrote:

In K&R, that's strcpy "pointer version 3"
[color=darkred]
> Well, to each their own.
> I would probably use a call to strcpy(); much faster.
>
> Unless I'm implementing strcpy(), of course.
> Then I'd probably use assembler.
>
> In the unlikely case I have to implement strcpy() in standard C, I'd
> probably use
>
> while ((*dest++ = *source++) != '\0');

An equality expression with three side effects,
is a little too busy for my taste.

I'm more partial to:

do {
*dest = *source++;
} while (*dest++ != '\0');

--
pete

Skarmander

2006-06-27, 6:56 pm

pete wrote:
> Skarmander wrote:
>
>
> In K&R, that's strcpy "pointer version 3"
>
>
> An equality expression with three side effects,
> is a little too busy for my taste.
>
> I'm more partial to:
>
> do {
> *dest = *source++;
> } while (*dest++ != '\0');
>
I thought about this, but in the end, the urge to keep the symmetry of the
++ operators was just too strong to ignore.

If I really wanted to split up all the side effects, of which I'm no great
fan either, I'd go all the way:

for (;;)
*dest = *source*;
if (*source == '\0') break;
++dest; ++source;
}

Loop with test in the middle, discussed in another thread. It's a small group.

S.

Skarmander

2006-06-27, 6:56 pm

Skarmander wrote:
> pete wrote:
> I thought about this, but in the end, the urge to keep the symmetry of
> the ++ operators was just too strong to ignore.
>
> If I really wanted to split up all the side effects, of which I'm no
> great fan either, I'd go all the way:
>
> for (;;)
> *dest = *source*;

Typo, obviously. Some newsreaders will helpfully *highlight* it. :-)

S.

pete

2006-06-27, 6:56 pm

Skarmander wrote:
>
> Skarmander wrote:
[color=darkred]
>
> Typo, obviously. Some newsreaders will helpfully *highlight* it. :-)

I avoid break statements when it's not too hard to do.

I also prefer

for (rc = getc(fp); rc != EOF; rc = getc(fp))

to

while ((rc = getc(fp)) != EOF)

I guess it's not necessarily the "three side effects"
in an equality expression that I don't like.

--
pete

Eric Sosman

2006-06-27, 6:56 pm

pete wrote On 06/27/06 16:35,:
>
> I also prefer
>
> for (rc = getc(fp); rc != EOF; rc = getc(fp))
>
> to
>
> while ((rc = getc(fp)) != EOF)

There's no point arguing with Gus, but one reason
to prefer the latter form is that you type the expression
only once instead of twice. That's not too bad when the
expression is a simple as this one, but even so it hands
a human reader the job of verifying that the expressions
are identical -- or, if they're not, whether they're
intentionally or accidentally different:

for (rc = getc(fp); rc != EOF; rc = fgetc(fp))

for (rc = getc(conn.fp); rc != EOF; rc = getc(fp))

for (rc = getc(fp); rc != EOF; r = getc(fp))

--
Eric.Sosman@sun.com

Skarmander

2006-06-27, 6:56 pm

pete wrote:
> Skarmander wrote:
>
>
> I avoid break statements when it's not too hard to do.
>
There's just no pleasing you.

> I also prefer
>
> for (rc = getc(fp); rc != EOF; rc = getc(fp))
>
Oh no! A duplicated statement! *faints*

Just kidding. But I don't imagine many programmers write this.

> to
>
> while ((rc = getc(fp)) != EOF)
>
> I guess it's not necessarily the "three side effects"
> in an equality expression that I don't like.
>
It doesn't win any beauty contests, but it's C. It wasn't meant to win
beauty contests. C's maxim is "brevity is... wit", and in that respect it's
a very witty language.

S.

pete

2006-06-27, 6:56 pm

Eric Sosman wrote:
>
> pete wrote On 06/27/06 16:35,:
>
> There's no point arguing with Gus, but one reason
> to prefer the latter form is that you type the expression
> only once instead of twice. That's not too bad when the
> expression is a simple as this one, but even so it hands
> a human reader the job of verifying that the expressions
> are identical -- or, if they're not, whether they're
> intentionally or accidentally different:
>
> for (rc = getc(fp); rc != EOF; rc = fgetc(fp))
>
> for (rc = getc(conn.fp); rc != EOF; rc = getc(fp))
>
> for (rc = getc(fp); rc != EOF; r = getc(fp))

Good point!

--
pete

Sponsored Links