Thread: failed return value (non-zero); code changed as per suggestion, still have question

Here's the same program, with altered code, as per suggestion by AndrewHunter and tabstop.

Now, here's why I'm still puzzled: when I run the program, again, everything seems to do what it's supposed to do, up until after the last function is called in main. I prove this by adding cin.get() to pause after the last function.

I've added testing the entire way, to prove to myself that each "copying" step of the set() function is successful. The tests seem to indicate that this is the case.

So seriously... I realize my novice status dictates automatically that my code will be sub par, but can anyone actually explain why the program still errors out at the very end? I just want to understand what I'm doing wrong. Try running the program and see... and for anyone who missed the last thread, yes, I realize I'm using C code in C++. Regardless of purity of standard, the program should still operate without the return value failure (that's why I'm trying to figure out exactly what I'm doing wrong).

Code:

#include <iostream>
using namespace std;

#include <cstring>
struct stringy {
    char * str;
    int ct;
    };

void set(stringy & x, char * y);
void show(stringy & a, int b = 1);
void show(const char * c, int d = 1);

int main()
{
   stringy beany;

   char testing[] = "Reality isn't what it used to be.";

   set(beany, testing);

   show(beany);
   show(beany, 2);
   testing[0] = 'D';
   testing[1] = 'u';
   show(testing);
   show(testing, 3);
   show("Done!");

   cout << "testing[14]: " << testing[14] << "\n";     // tests original string index
   cout << "beany.str[14]: " << beany.str[14] << "\n"; // tests copied string index

   cout << "testing[6]: " << testing[6] << "\n";       // test of a different index, just to be sure
   cout << "beany.str[6]: " << beany.str[6] << "\n";   // test of a different index, just to be sure

   cout << "testing: " << testing << "\n";      // verifying entire string still intact
   cout << "beany.str: " << beany.str << "\n";  // verifying entire copied string is still intact

   cin.get();        /// this is here to prove program runs smoothly until return fails!!!

   return 0;
}

void set(stringy & x, char * y)
{
   size_t length = strlen(y) + 1;

   char * space = new char[length];

   cout << "y[14]: " << y[14] << "\n";         // verifying testing string with random index
   cout << "space[14]: " << space[14] << "\n"; // verifying space currently has garbage values
   cout << "x.str[14]: " << x.str[14] << "\n"; // verifying sure beany.str currently has garbage values

   space[length] = '\0';

   for (int i = 0; i < length; i++)
        space[i] = y[i];

   cout << "y[14]: " << y[14] << "\n";         // verifying testing string with random index
   cout << "space[14]: " << space[14] << "\n"; // verifying space received proper values
   cout << "x.str[14]: " << x.str[14] << "\n"; // verifying sure beany.str currently has garbage values

   space[length] = '\0';

   strcpy(x.str, space);

   cout << "y[14]: " << y[14] << "\n";         // verifying testing string with random index
   cout << "space[14]: " << space[14] << "\n"; // verifying space received proper values
   cout << "x.str[14]: " << x.str[14] << "\n"; // veryfying beany.str received proper values

   cout << "testing: " << y << "\n";    // verifying testing string is still intact
   cout << "space: " << space << "\n";  // veriyfing space string was copied correctly
   cout << "x.str: " << x.str << "\n";  // verifying beany.str string was copied correctly

   delete space;

}

void show(stringy & a, int b)
{
   for (int i = 0; i < b; i++)
      cout << a.str << "\n";
}

void show(const char * c, int d)
{
   for (int i = 0; i < d; i++)
      cout << c << "\n";
}

Originally Posted by Alexandre

I tested your code by adding a simple "x.str = new char[length];" and there is no more segmentation fault nor are there compiler warnings or errors. My guess is that your compiler being in debug mode has protected you from unexpected behavior until the end of excution. Most probably if you select release mode it will not be so easy on you. I will chime in with the others though and tell you that this type of code should be avoided as much as possible in C++. I almost never use dynamic memory allocation because most of the time there is an STL container that will do the job even better than I could. In this case, a simple use of std::string would solve all your problems. You could even push it and use std::list or std::vector instead of dynamic memory allocation.

Thanks for this info!!! I think I need to make some adjustments to my debugger messages, so they include notification of segmentation faults. I actually agree with all of you; I'd rather be using pure C++. I'm being patient with the book, because it was written with people in mind who are already familiar with coding in C. I looked ahead, and clearly the things I'm doing now are designed to prepare me for the transition (sort of like laying the groundwork for comparison, instead of just blind memorization).

Anyway, this is great! I think I have the tools now to attack this properly.

Ok, some notes:

Code:

int main()
{
   stringy beany;

   char testing[] = "Reality isn't what it used to be.";

   set(beany, testing);

   show(beany);
   show(beany, 2);
   testing[0] = 'D';
   testing[1] = 'u';
   show(testing);
   show(testing, 3);
   show("Done!");

[edit]
This is incorrect, see comments below
The items in red above are where you are trying to modify a string literal. You are not allowed to do this, and it results in undefined behavior. Here is a short guide on what undefined behavior is.
[/edit]
Below is a sample code that shows you how to do what I was talking about. Note that you do not need to use for loops to print out even a C style string, as std::cout takes care of this for you. Even in C you would just use the printf function.

FYI: In cases like this, since it is the same topic and program as your other thread you would continue with the same thread vice starting a new one.

Code:

#include <iostream>
#include <cstring>

struct string{
	char *mystring;
	int size;
};

void set(string &, char*);

int main(void){

	string stringtest; //create struct object
	char string[]="This is a string literal";

	//set our string object
	set(stringtest, string);

	std::cout<<"String is: >"<<stringtest.mystring <<"< with length "<<stringtest.size;

	std::cin.get();
	return(0);
}
void set(string &stringref, char* stringtocopy){

	//get string size and store it in size member
	stringref.size = strlen(stringtocopy);
	
	//allocate space for new string plus 1 for '/0'
	stringref.mystring = new char[stringref.size + 1];

	//copy string literal to our string object
	memcpy(stringref.mystring, stringtocopy,stringref.size);
	//append null to string, remember C strings start a 0
	stringref.mystring[stringref.size] = '\0';
}

Originally Posted by AndrewHunter

The items in red above are where you are trying to modify a string literal.

It looks like testing is an array of char that was initialised with a string literal, not a string literal.

Originally Posted by AndrewHunter

Ok, some notes:

The items in red above are where you are trying to modify a string literal. You are not allowed to do this, and it results in undefined behavior. Here is a short guide on what undefined behavior is.

Below is a sample code that shows you how to do what I was talking about. Note that you do not need to use for loops to print out even a C style string, as std::cout takes care of this for you. Even in C you would just use the printf function.

FYI: In cases like this, since it is the same topic and program as your other thread you would continue with the same thread vice starting a new one.

Re: the string literal... I thought that initializing a character array string literal (as opposed to initializing a pointer-to-char string literal) made modification of individual array elements compliant with the C++ (and C) standards.

Re: the new thread: sorry about that. I'll try not to do that again.

Re: oh man... after looking at your code, I realize I made a cluster-you-know-what out of one of the exercise instructions for set(). Sheesh. I looked back at the book after seeing your code, and I realized I misinterpreted the instruction by mistakenly adding an needless extra step instead of just allocating memory for beany.str (with x.str). My "space" pointer-to-char nonsense was completely worthless in this scenario. Hence the initial replies by you and tabstop.

Thanks much for all your help!!!

Originally Posted by laserlight

It looks like testing is an array of char that was initialised with a string literal, not a string literal.

Originally Posted by spongefreddie

Re: the string literal... I thought that initializing a character array string literal (as opposed to initializing a pointer-to-char string literal) made modification of individual array elements compliant with the C++ (and C) standards.

Oops, missed that when I looked through. Laser and you are both correct. You initialized it as an array of characters.

Originally Posted by spongefreddie

Re: the new thread: sorry about that. I'll try not to do that again.

No worries, just letting you know.

Originally Posted by spongefreddie

Re: oh man... after looking at your code, I realize I made a cluster-you-know-what out of one of the exercise instructions for set(). Sheesh. I looked back at the book after seeing your code, and I realized I misinterpreted the instruction by mistakenly adding an needless extra step instead of just allocating memory for beany.str (with x.str). My "space" pointer-to-char nonsense was completely worthless in this scenario. Hence the initial replies by you and tabstop.

Thanks much for all your help!!!

We all make mistakes, the important thing is to learn from them. I believe what the author is doing with these problems is building you up to make your own string class.