Is there anything wrong with the code?

[dullboy]

The code in the following, could you point out what is wrong? Why is it wrong? Thanks for your inputs.



void reverseString(const char* str1, char* str2)
{
int len = strlen(str1);

str2 = new char[len+1];

char* p = const_cast<char*>(str1) + len - 1;

while(p>=str1)
{
*str2++ = *p--;
}

*str2 = '\0';

}

int main()
{

const char* s1 = "Tony";

char* s2;

reverseString(s1, s2);

cout<<s2<<endl;

return 0;
}

[Paul McKenzie]

The code in the following, could you point out what is wrong? Why is it wrong? Thanks for your inputs.Is this a homework question?

I say that since there are obvious things wrong with it, but you haven't specified even one of the many things wrong with the code.

Regards,

Paul McKenzie

[dullboy]

No, this is not a homework question. Sorry I didin't specify where it is wrong because I thought if you run this program, you will find out. Basically, there is an exception saying that "s2 is being used without being defined.". But I thought s2 is pointed to str2 which is allocated memory within function reverseString. Thanks for your inputs.
Is this a homework question?

I say that since there are obvious things wrong with it, but you haven't specified even one of the many things wrong with the code.

Regards,

Paul McKenzie

[VladimirF]

You pass char* s2; to your function reverseString() by value; that function overwrites your passed-in value with a pointer to newly allocated memory (that will leak at the end of that function); calling party is not aware of this action - the value of its s2 did not change and still is uninitialized..

[Philip Nicoletti]

1) You do a "new[]" , but no "delete[]"

2) You pass s2 by value to reverseString. In reverseString you change
its value (when you do the new[]), but that will not be seen back in main()

You need to pass : char ** or char *&

3) Once you do the "new[]" , you should not change what str2 points to.

[kempofighter]

No, this is not a homework question. Sorry I didin't specify where it is wrong because I thought if you run this program, you will find out. Basically, there is an exception saying that "s2 is being used without being defined.". But I thought s2 is pointed to str2 which is allocated memory within function reverseString. Thanks for your inputs.

Not sure what you mean by exception. The code didn't crash for me or throw any exceptions. It didn't work but it didn't crash either. I did get a warning that s2 is used prior to being set, which is true. You don't initialize the pointer to 0 prior to passing it to the function. Manipulating the pointers is a bad idea. At the end, s2 will be null because you keep moving the address pointed to by it and it will ultimately end up pointing directly to the NULL. You need to leave the pointers where they are at and just use array notation so that you are modifying the array without changing what the char* is pointing to.

[dullboy]

Thanks for VladimirF, Philip and Kempofighter. You guys' opinions are very helpful. Now I didn't change the value s2 points to so I need to pass char** s2 or char* &s2. But I still don't understand why s2 needs to be initailized before I pass it to reverseString? Thanks for your inputs.

[Paul McKenzie]

No, this is not a homework question.Well one of the obvious things is that you used new[] without using delete[]. This is, or I thought would be the most glaring thing in the program that should have been noticed by anyone with familiarity with C++.

That's why the original question smacked of a homework question.

Regards,

Paul McKenzie

[Paul McKenzie]

Thanks for VladimirF, Philip and Kempofighter. You guys' opinions are very helpful. Now I didn't change the value s2 points to so I need to pass char** s2 or char* &s2. But I still don't understand why s2 needs to be initailized before I pass it to reverseString?It does not need to be initialized. Note that the uninitialized variable is just a warning, it isn't an error.

Anytime you pass anything by value, and you haven't initialized what you passed, the compiler will give you a warning that you're passing a variable that is not initialized. Regardless of whether that entity is an int, a double, a char, a pointer, a pointer to a pointer, it doesn't matter as all of these are passed by value. If the entity isn't initialized, you get the warning.

Regards,

Paul McKenzie

[active2volcano]

The design of the function have some problem!

First, you have new the heap memory in the function , the motion didn't gave any hint to the user!Any one who use that function didn't know he had secretly new a memory!He will forget to delete the memory!
You are that kind!

Second, the parament table also seem to weird!
At the same time , the function can't process the new's exception!If the new is error, str = 0;It point to position of Managed by the OS!
while execute the code as this :

*str2++ = *p--;


Third, using const_cast often means a bad design!

The design is the function design!

[Graham]

Third, using const_cast often means a bad design!
Exacerbated by const_cast being unnecessary, since *p is not modified.

[JohnW@Wessex]

As somebody will undoubtedly mention eventually, if you can use the STL then it all becomes dead simple.

#include <string>
#include <algorithm>
#include <iostream>

using namespace std;

int main()
{
const string s1("Tony");

string s2(s1);

reverse(s2.begin(), s2.end());

cout << s2 << endl;

return 0;
}

[dullboy]

But if you run my original code in visual studio 2005, it will CRASH. So it looks like here uninitailized s2 is more than a warning. Thanks for your inputs.
It does not need to be initialized. Note that the uninitialized variable is just a warning, it isn't an error.

Anytime you pass anything by value, and you haven't initialized what you passed, the compiler will give you a warning that you're passing a variable that is not initialized. Regardless of whether that entity is an int, a double, a char, a pointer, a pointer to a pointer, it doesn't matter as all of these are passed by value. If the entity isn't initialized, you get the warning.

Regards,

Paul McKenzie

[VladimirF]

But if you run my original code in visual studio 2005, it will CRASH. So it looks like here uninitailized s2 is more than a warning. Thanks for your inputs.It will most likely crash under ANY studio because you dereference uninitialized pointer.
You are mixing two things:
1. You have a compiler warning that you are using unitialized variable:warning C4700: uninitialized local variable 's2' usedYou only get ONE such warning per variable.
2. You have a run-time error:
Run-Time Check Failure #3 - The variable 's2' is being used without being initialized.when the run-time actually determins that you are using that uninitialized variable. You get this for every occurance. Consider this fragment: char* s2;
char* s3 = s2;
char* s4 = s2;
Although you get two run-time failures, there is no crash (yet).
However, this line cout<<s2<<endl;will crash (most likely).

[dullboy]

Regarding 2), if I pass char*, the program still works. Here is the updated code.



int reverseString(const char* str1, char* str2)
{
char* p;
int len;

len = strlen(str1);

p =const_cast<char*>(str1)+len-1;

while(p>=str1)
{
*str2++ = *p--;
}

*str2 = '\0';
return 1;
}

int main()
{
const char* str = "My name is dullboy";

char* str2;


str2 = new char[100];

reverseString(str, str2);

cout<<str2<<endl;

delete[] str2;
return 0;
}


Check the defintion of reverseString,
int reverseString(const char* str1, char* str2). I passed char* not char** or char* & and I still get correct str2. Thanks for your inputs.
1) You do a "new[]" , but no "delete[]"

2) You pass s2 by value to reverseString. In reverseString you change
its value (when you do the new[]), but that will not be seen back in main()

You need to pass : char ** or char *&

3) Once you do the "new[]" , you should not change what str2 points to.

[Philip Nicoletti]

Regarding 2), if I pass char*, the program still works. Here is the updated code.



int reverseString(const char* str1, char* str2)
{
char* p;
int len;

len = strlen(str1);

p =const_cast<char*>(str1)+len-1;

while(p>=str1)
{
*str2++ = *p--;
}

*str2 = '\0';
return 1;
}

int main()
{
const char* str = "My name is dullboy";

char* str2;


str2 = new char[100];

reverseString(str, str2);

cout<<str2<<endl;

delete[] str2;
return 0;
}


Check the defintion of reverseString,
int reverseString(const char* str1, char* str2). I passed char* not char** or char* & and I still get correct str2. Thanks for your inputs.


In your original post (which is what I was commenting on), you did the
"new" in reverseString , not in main.

[JohnW@Wessex]

const_cast !!!
Completely unnecessary.
int reverseString(const char* str1, char* str2)
{
const char* p;
int len;

len = strlen(str1);

p =str1+len-1;

while(p>=str1)
{
*str2++ = *p--;
}

*str2 = '\0';
return 1;
}