Is this str_rev() ok?

[ballpointpenthief]

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 = "Dontchajustwishthisstringwouldgetreversed";
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]

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]

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]

On 27 Jun 2006 11:00:34 -0700, "ballpointpenthief"
<Ma*************@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 = "Dontchajustwishthisstringwouldgetreversed";
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/~a...FAQ-acllc.html

[Roberto Waltman]

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]

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

"ballpointpenthief" <Ma*************@gmail.com> wrote in message
news:11*********************@b68g2000cwa.googlegro ups.com...

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:

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]

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]

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:

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

y = malloc(arrLength);
[...]
tmp = &y[arrLength-1];
while (*--tmp = *x++);

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.

--
Er*********@sun.com

[Malcolm]

"Frederick Gotham" <fg*******@SPAM.com> wrote

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!

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

[Frederick Gotham]

Malcolm posted:

"Frederick Gotham" <fg*******@SPAM.com> wrote

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!

You are a clever person.
Others are baffled.

While intelligence may equate to potential to be a good programmer, it
all comes down to experience. Practise makes perfect.

I don't throw around *p++ because I'm intelligent, but because I'm
familiar with it and know exactly what it does.

Also, on a lighter note, I'd sooner drink bleach than dumb-down my code
in order for a novice to understand it. : )
--

Frederick Gotham

[pete]

ballpointpenthief wrote:

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;
}

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]

Frederick Gotham wrote:

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.

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]

Skarmander wrote:

Frederick Gotham wrote:

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!

In K&R, that's strcpy "pointer version 3"
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]

pete wrote:

Skarmander wrote:

Frederick Gotham wrote:

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!

In K&R, that's strcpy "pointer version 3"

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');

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]

Skarmander wrote:

pete wrote:

Skarmander wrote:

Frederick Gotham wrote:

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!

In K&R, that's strcpy "pointer version 3"

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');

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]

Skarmander wrote:

Skarmander wrote:

pete wrote:

Skarmander wrote: 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');

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. :-)

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]

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))

--
Er*********@sun.com

[Skarmander]

pete wrote:

Skarmander wrote:

Skarmander wrote:

pete wrote:
Skarmander wrote: 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');

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. :-)

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]

Eric Sosman wrote:

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))

Good point!

--
pete