String Reverse Question

[Rakesh]

Hi,
I have this function to reverse the given string.
I am just curious if that is correct and there could be better way of
doing it / probable bugs in the same.

The function prototype is similar to the one in any standard C
library.


<---- Code starts -->

char * my_strrev(char * mine) {
static char * p = NULL; // To make sure that the pointer is not
lost on return.
char * q;
int len;
int i;

len = strlen(mine);
p = new char[len + 1];
// 1 for '\0' character

q = mine;
i = len - 1;
while (*q) {
*(p + i) = *q++;
i--;
}
*(p + len ) = '\0';
return p;
}

<-- Code Ends -->

- Rakesh.

 

[Régis Troadec]

Hi,
Hi,

I have this function to reverse the given string.
I am just curious if that is correct and there could be better way of
doing it / probable bugs in the same.

The function prototype is similar to the one in any standard C
library.


<---- Code starts -->

char * my_strrev(char * mine) {
static char * p = NULL; // To make sure that the pointer is not
lost on return.
char * q;
int len;
int i;

len = strlen(mine);

mine could be NULL and cause a segfault. Be also careful that mine is null
terminated.

You can put this above the strlen call :
if (mine==NULL) return NULL;

p = new char[len + 1];

new ?? it looks like C++, not C. Use malloc, calloc, or realloc instead.

p = malloc(len+1);

if (p)
{

// 1 for '\0' character

q = mine;
i = len - 1;
while (*q) {
*(p + i) = *q++;
i--;
}
*(p + len ) = '\0';

}

 

[Rakesh]

mine could be NULL and cause a segfault. Be also careful that mine is null
terminated.

Oh Yeah - Thanks a ton. This is very important. But just a design q.
here - am i supposed to handle this one / should i leave it to the
user of the library to make sure that NULL is not passed to this.
Which one seems the best.

You can put this above the strlen call :
if (mine==NULL) return NULL;

p = new char[len + 1];

new ?? it looks like C++, not C. Use malloc, calloc, or realloc instead.

Oops ! My mistake . Of late, I had been doing a lot in C++ and
hence the problem.

Thanks for your comments, Régis

 

[Christopher Benson-Manica]

char * my_strrev(char * mine) {

const char *mine

static char * p = NULL; // To make sure that the pointer is not
lost on return.

1) Don't post C++ style comments to comp.lang.c (more later)
2) Misplaced concern - the pointer value is returned and thus is
not lost.

char * q;

Harmless but unnecessary - you can simply use mine rather than
declaring another variable.

int len;
int i;

len = strlen(mine);
p = new char[len + 1];

This (new) is C++. Don't post it here. ITYM

p=malloc( len*sizeof(*p)+1 );

// 1 for '\0' character

You did well to remember it.

 

[Christopher Benson-Manica]

Oh Yeah - Thanks a ton. This is very important. But just a design q.
here - am i supposed to handle this one / should i leave it to the
user of the library to make sure that NULL is not passed to this.
Which one seems the best.

Most of the standard string.h functions dislike NULL pointers, so
having your code do likewise (and documenting that fact) seems
reasonable. (I missed this issue - oops.)

 

[Martin Ambuhl]

Rakesh <[email protected]> spoke thus:

1) Don't post C++ style comments to comp.lang.c (more later)

The real concern is not that "//..." are C++ style comments. As of C99,
they are C comments as well. The problem is shown by your quotation;
the comment is split by either his posting software or your reading
software. This makes the code uncompilable without going through and
pasting comments back together. This problem suggests that not only
posters to comp.lang.c but also posters to comp.lang.c++ ought not use
that style comment. After all, "/* ... */" comments are still legal in C++.

 

[Al Bowers]

Hi,
I have this function to reverse the given string.
I am just curious if that is correct and there could be better way of
doing it / probable bugs in the same.

The function prototype is similar to the one in any standard C
library.

In addition to the excellent help you good from others, I would
like to add the following.

<---- Code starts -->

char * my_strrev(char * mine) {

Since the string argument will not be altered, I would make it:
char *my_strrev(const char *mine)

static char * p = NULL; // To make sure that the pointer is not
lost on return.
char * q;
int len;

The strlen function returns type size_t. Although your int type for
len may not fail, it would be best to make the this:
size_t len;

int i;

len = strlen(mine);
p = new char[len + 1];
// 1 for '\0' character

q = mine;
i = len - 1;

Opps! You need to consider how you want to handle the
situation, should a user of the function call it with an
empty string,ie., my_strrev("");. Here len will have
the value of 0, and in the expression i = len - 1;, i
will have the value of -1.

while (*q) {

*(p + i) = *q++;

Here is the "Opps!". With i have value of -1, the code
*(p + i) becomes *(p - 1)

i--;
}
*(p + len ) = '\0';
return p;
}

I would suggest that you put in the function a check testing
len for 0 and take appropriate action.

Example:

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

char *my_strrev(const char *str)
{
char *rev = NULL,*s1;
size_t len = strlen(str);

if(len != 0 && (rev = malloc(len+1)) != NULL)
for(*(rev+len) = '\0',s1=(char *)str;len;s1++,len--)
*(rev+(len-1)) = *s1;
return rev;
}

int main(void)
{
char *s,str[] = "This is a long string";

if((s = my_strrev("This is a long string")) != NULL)
printf("The string \"%s\"\nreversed is \"%s\"\n",str,s);
else printf("The string \"%s\" was not reversed\n",str);
free(s);
return 0;
}

 

[Robert Bachmann]

Al Bowers wrote:
[snip]

char *my_strrev(const char *str)
{
char *rev = NULL,*s1;
size_t len = strlen(str);

if(len != 0 && (rev = malloc(len+1)) != NULL)

Why (len != 0)?
If I invoke my_strrev("") I would expect it
to return a pointer to '\0' and not a NULL-pointer.

for(*(rev+len) = '\0',s1=(char *)str;len;s1++,len--)
*(rev+(len-1)) = *s1;
return rev;
}

Best regards,
Robert Bachmann

 

[Joe Wright]

Hi,
I have this function to reverse the given string.
I am just curious if that is correct and there could be better way of
doing it / probable bugs in the same.

The function prototype is similar to the one in any standard C
library.


<---- Code starts -->

char * my_strrev(char * mine) {
static char * p = NULL; // To make sure that the pointer is not
lost on return.
char * q;
int len;
int i;

len = strlen(mine);
p = new char[len + 1];
// 1 for '\0' character

q = mine;
i = len - 1;
while (*q) {
*(p + i) = *q++;
i--;
}
*(p + len ) = '\0';
return p;
}

<-- Code Ends -->

- Rakesh.

You're kidding, right?

char *revstr(char *s) {
char t, *b = s, *e = s + strlen(s) -1;
if (s) while (b < e) t = *b, *b++ = *e, *e-- = t;
return s;
}

Maybe it's more difficult in C++. :-(

 

[Ben Pfaff]

char *revstr(char *s) {
char t, *b = s, *e = s + strlen(s) -1;
if (s) while (b < e) t = *b, *b++ = *e, *e-- = t;
return s;
}

Maybe it's more difficult in C++. :-(

In C++ it's easier:
std::reverse(s, strchr(s, '\0'));

 

[James McIninch]

<posted & mailed>

The code you posted won't compile with a C compiler. Here is a valid C
version:

#include <stdlib.h>
#include <string.h>
char * strrev (const char *s)
{
char * p;
int i, j;
j = strlen(s)
if (p = calloc(1, j+1))
for (i=0; i<j; ++i)
p = s[j-i];
return p;
}

Hi,
I have this function to reverse the given string.
I am just curious if that is correct and there could be better way of
doing it / probable bugs in the same.

The function prototype is similar to the one in any standard C
library.


<---- Code starts -->

char * my_strrev(char * mine) {
static char * p = NULL; // To make sure that the pointer is not
lost on return.
char * q;
int len;
int i;

len = strlen(mine);
p = new char[len + 1];
// 1 for '\0' character

q = mine;
i = len - 1;
while (*q) {
*(p + i) = *q++;
i--;
}
*(p + len ) = '\0';
return p;
}

<-- Code Ends -->

- Rakesh.

 

[Richard Delorme]

Joe Wright a écrit :

char *revstr(char *s) {
char t, *b = s, *e = s + strlen(s) -1;
if (s) while (b < e) t = *b, *b++ = *e, *e-- = t;
return s;
}

Just some questions about this code:

1) What happens if s is a null pointer? Is strlen(s) supposed to work?
2) If e value is (s - 1), isn't the comparison (b < e) an undefined
behaviour, as e is no more pointing to an object or one element past
that object?

 

[Barry Schwarz]

<posted & mailed>

The code you posted won't compile with a C compiler. Here is a valid C
version:

Only for strange definitions of valid. It neither compiles nor
performs after the syntax error is fixed.

#include <stdlib.h>
#include <string.h>
char * strrev (const char *s)

Names starting with str followed by a lower case letter are reserved.

{

Let's assume that s points to an array containing "ab".

char * p;
int i, j;
j = strlen(s)

j would be set to 2 if there were a semicolon at the end of the
statement.

if (p = calloc(1, j+1))
for (i=0; i<j; ++i)

For the first iteration through the loop, i is 0.

p = s[j-i];

For the first iteration, this is p[0] = s[2]. We already know that
s[2] is '\0' so p will forever point to a string of length 0.

Additionally, since i never equals j, s[0] is never copied to p.

return p;
}

<<Remove the del for email>>

 

[Barry Schwarz]

Joe Wright a écrit :

Just some questions about this code:

1) What happens if s is a null pointer? Is strlen(s) supposed to work?

strlen does not support receiving a NULL pointer. Behavior would be
undefined at this point.

2) If e value is (s - 1), isn't the comparison (b < e) an undefined
behaviour, as e is no more pointing to an object or one element past
that object?

This can only happen is s points to a string whose first char is '\0'
but if that happens then you are correct about the undefined behavior.



<<Remove the del for email>>

 

[Régis Troadec]

"Régis Troadec" <[email protected]> wrote in message

Oh Yeah - Thanks a ton. This is very important. But just a design q.
here - am i supposed to handle this one / should i leave it to the
user of the library to make sure that NULL is not passed to this.
Which one seems the best.

It depends on your own requirements : would you like the user of your
functions take care about null pointers and memory handling or not ?
As Christopher said, most of standard functions of <string.h> need to pay
careful attention with null pointers. I think it's fine to code in the same
way as well I think it's fine to prevent any memory access violation. If it
were for business, I would implement my functions according to the given
requirements. For my personal stuff, I always handle null pointers.

Finally, I missed to propose you my implementation of string reversing, here
it is :

<code>

char * my_strrev(const char * src)
{
size_t len, i, j;
char * dst;

if (src==NULL) return NULL;

len = strlen(src);

if ((dst = malloc(len+1)) != NULL)
{

if (len % 2) dst[len/2] = src[len/2];
for(i=0, j=len-1; i<len/2; ++i, --j)
{
dst = src[j]; dst[j] = src;
}
dst[len] = '\0';
}

return dst;
}

</code>

Regis

You can put this above the strlen call :
if (mine==NULL) return NULL;

p = new char[len + 1];

new ?? it looks like C++, not C. Use malloc, calloc, or realloc instead.

Oops ! My mistake . Of late, I had been doing a lot in C++ and
hence the problem.

Thanks for your comments, Régis

 

[Keith Thompson]

char *revstr(char *s) {
char t, *b = s, *e = s + strlen(s) -1;
if (s) while (b < e) t = *b, *b++ = *e, *e-- = t;
return s;
}

You're checking whether s is null *after* passing it to strlen().

 

[Keith Thompson]

This (new) is C++. Don't post it here. ITYM

p=malloc( len*sizeof(*p)+1 );

(where p was declared as a char*).

If we're going to assume that p is a char*, the sizeof is superfluous;
it's simpler and clearer to write:

p = malloc(len + 1);

That's a reasonable assumption given that we're allocating extra space
for a '\0' character.

On the other hand, if we want to allow for the possibility that the
declaration of p could be changed so it points to something bigger
than one byte (perhaps a wchar_t), we'd want to use:

p = malloc(len * (sizeof *p + 1));

to allocate one additional object rather than one additional byte.

 

[Régis Troadec]

"Keith Thompson" <[email protected]> a écrit dans le message de

Hi,

[snipped]

On the other hand, if we want to allow for the possibility that the
declaration of p could be changed so it points to something bigger
than one byte (perhaps a wchar_t), we'd want to use:

p = malloc(len * (sizeof *p + 1));

to allocate one additional object rather than one additional byte.

I guess you meant p = malloc( (len+1)*sizeof*p ); to allocate one
additional object.

Regards,
Regis

 

[Keith Thompson]

It depends on your own requirements : would you like the user of your
functions take care about null pointers and memory handling or not ?
As Christopher said, most of standard functions of <string.h> need to pay
careful attention with null pointers.

Actually, most of the standard functions in <string.h> can ignore the
issue of null pointers altogether. Passing a null pointer to strlen()
invokes undefined behavior.

If you're going to have your own string functions check for null
pointers, you need to decide how to handle the error. Since C doesn't
have exceptions, there's no obvious way to indicate the error.

 

[Al Bowers]

Al Bowers wrote:
[snip]

char *my_strrev(const char *str)
{
char *rev = NULL,*s1;
size_t len = strlen(str);

if(len != 0 && (rev = malloc(len+1)) != NULL)

Why (len != 0)?
If I invoke my_strrev("") I would expect it
to return a pointer to '\0' and not a NULL-pointer.

If there is an empty string argument on the above defined function,
then there would not be a dynamic allocation and NULL is returned.

If the OP purpose was to do as you expected, then the OP
code will fail. The example was a demo of correcting this flaw,
without changing the OP's use of a dynamic allocation.

I too would do what you expect. But doing this in a function, which
would dynamically allocate memory for no useful purpose (there
are no characters to reverse), is a waste of resources.
That is not to consider the resources of deallocation should
you feel the need.

To do what you expect, I would design the function to have
two char point arguments, one pointing to the source string, and
the other pointing to storage that is declared/allocated by the
calling function. Therefore, there would be no need for the
my_strrev function to make allocations. The return value will
simply point to the dest storage where a empty string will reside.

I'm surprised that you did not catch the boner in
function main. .