dynamic memory noob

[leo235]

#include "stdafx.h"
#include "product.h"

int product::pricemid = 0;
int product::number_of_products = 0;


int _tmain(int argc, _TCHAR* argv[])
{
	int prod = 0;
	int a = 0,b = 0,c = 0,d = 0,e = 0,f = 0;
		
	 
	product *p = new product [100];

	p[0].set_it (20,"zwanzigeuroschein",100,"Geld");
	p[1].set_it (40,"zweizwanzigeuroschein",100,"Geld");
	p[2].set_it (60,"dreizwanzigeuroschein",100,"Geld");
	
	
	for (int i=0; i != 1; )
	{
		    
		char *name1 = new char[40] ;
	    char *name2 = new char [40];
	    char *name3 = new char [40];
		
		cout << "was willst du tun?" << endl;
		cin >> name1;
		cout << endl;
		
			cout << "was ist der preis? (nur eine zahl)" << endl;
			cin >> a;
			cout << " was ist der name des Produktes? " << endl;
			cin >> name2;
			cout << "wie hoch ist die Produktqualität? " << endl;
			cin>> b;
			cout << " was ist der Produkttyp?" << endl;
			cin >> name3;
			p[prod].set_it (a,name2,b,name3);
			cout << "1 für ende 0 für weiter";
			cin >> i;
			p[prod].printprice();
			prod++;

			if (prod >1)
			{p[(prod - 1)].printprice();}
			
			
			
			
			name1 = NULL;
			delete [] name1;
			
			delete [] name2;
			//name2 = NULL;

			delete [] name3;
			//name3 = NULL;
		
	}

	
	
	
	
	cin >> a;
	return 0;


}

and

#include <iostream>
using namespace std;

class product {
public :
	

	char *name ;
	char *type;
	int price;
	int quality;
	int brand_recognition;
	int brand_loyalty;
	int player;
	//factory * manufacturer;
	int identifier;
		//SHOULD BE DIFFRENT FOR DIFFERENT PRODUCTS BUT ISNT THIS WAY
		static int pricemid ;
		static int number_of_products;
		

		void setprice (int);
		void printprice() ;
		product(int prc = 0,char *nm = "product" , int qlty = 0, char *type = "nix");
		void set_it(int prc = 0,char *nm = "product" , int qlty = 0, char *type = "nix");
		~product();
};

void product::setprice (int prc)
{
	this -> price = prc;
};

void product::printprice ()
{
	cout << "der Preis des Produktes" << this -> name << " ist " << this->price << endl;
	cout << " es ist vom typ " << this ->type << endl;
};

product::product (int prc, char *nm, int qlty, char *type)
{
	this -> name = new char[strlen(nm)];
	this -> type = new char[strlen(type)];
	this -> price = prc;
	//this -> name = nm; //instead:
	strcpy(this->name , nm);
	this ->quality = qlty;
	//this -> type = type;
	strcpy(this->type,type);
	this->number_of_products++;
	

};

void product::set_it(int prc ,char *nm  , int qlty , char *type )
{
	int i, o;
	this -> name = new char[strlen(nm)] ;
	this -> type = new char[strlen(type)];
	this -> price = prc;
	//this -> name = nm;
	strcpy(this->name , nm);
	this ->quality = qlty;
	//this -> type = type;
	strcpy(this->type,type);
	this->number_of_products++;
	for (i=0;i < number_of_products; i++)
	{
		o = 1;
	}
};

product::~product ()
{
	delete[] this -> name;
	this->name = NULL;
	delete[] this -> type;
	this->type = NULL;
};

Problems:

without the name1 = NULL; at the end of the loop I get an error about a failed asses.

Also p[prod].printprice(); and p[(prod-1)].printprice(); give the same result



THANKS

[laserlight]

I suggest that you use std::string. If you really want to practice dynamic memory allocation, write a string class.

[jnmacd]

this -> name = new char[strlen(nm)] ;

This does not allocate enough memory for

strcpy(this->name , nm);

Use std::string.

[leo235]

@jnmacd:
could you elaborate a little on that?
if I allocate 5 chars for a 5-char-word how is that not enough does strcpy somehow need some extra space?

@laserlight:
Im sorry but I expected this to be a peice of cake and now I refuse to let go of this way of doing it until I know all the things i did wrong and how to do them right. After that Ill probably rewrite the little code I have right now and use strings.

could you still try to answer the 2 original questions?
I know its a bit of the worse kind of work, but Ill repay you with nice words and magic that will make you happy and immortal.

[MrViggy]

Yes, you need to allocate one extra char for the terminating null character. So, for a 5 character word, you need to allocate a minimum of 6 characters.

Viggy

[Lindley]

Okay, all the issues I see.....

#include "stdafx.h"

stdafx.h is non-standard and frankly, not very useful for a project this size. You need to be working with dozens of source files at least before precompiled headers really help your compile times. I would suggest dropping the stdafx.h include and disabling precompiled headers in your project.

int product::pricemid = 0;
int product::number_of_products = 0;

Typically if these are required by product.h, then they should go in a file called product.cpp. That's not strictly necessary, but it helps keep things compartmentalized.

int _tmain(int argc, _TCHAR* argv[])

The _tmain and TCHAR stuff is frankly unnecessary and I don't know why Microsoft pushes it. The main function will always work when declared simply as int main(int argc, char**argv), regardless of the character set selected (Unicode or MBCS), and this signature makes the code less Microsoft-specific.

		char *name1 = new char[40] ;
//...
		cin >> name1;

This code is insecure. If a name longer than 39 characters is entered, it could corrupt memory. Worse, it could allow an attacker to execute any code they want via a buffer overflow attack. If you only have 40 bytes available, you need to use the mechanisms in the iostream library to make sure no more than 39 chars are read. One of the nice things about std::string is that it will automatically resize itself when needed, up to available memory limitations, so this problem doesn't exist.

By the way---since you're always allocating the same number of bytes anyway (40), why are you using dynamic memory here at all? A simple array on the stack would accomplish the same thing, with the above caveat.

			name1 = NULL;
			delete [] name1;

This won't do anything useful. Set it to NULL after deleting the pointer if you want. Although as I said, if you just put this array on the stack there would be no need for a delete.

#include <iostream>
using namespace std;

class product {

Don't put "using" statements in header files, at least at unrestricted scope.

		//SHOULD BE DIFFRENT FOR DIFFERENT PRODUCTS BUT ISNT THIS WAY
		static int pricemid ;

Perhaps each individual product should be a different class, all derived from the product class?

void product::setprice (int prc)
{
	this -> price = prc;
};

As I mentioned above, usually this sort of function definition should be in a source file like product.cpp, not in a header. A trivial function like this can be written in the header for better inlining, but in that case it is usually either explicitly declared inline or else written within the class definition.

If you tried to #include this header in multiple source files as it is now, you would probably get linker errors.

product::product (int prc, char *nm, int qlty, char *type)
{
	this -> name = new char[strlen(nm)];
	this -> type = new char[strlen(type)];
	this -> price = prc;
	//this -> name = nm; //instead:
	strcpy(this->name , nm);
	this ->quality = qlty;
	//this -> type = type;
	strcpy(this->type,type);
	this->number_of_products++;
};

As has been mentioned, you need to allocate one more char per C-style string.

However, there's a bigger problem. The "product" class is not safely copyable. Consider this code:

#include "product.h"
int main()
{
    product p(0,"Hello",1,"Goodbye");
    product p2 = p;
}

This will probably crash (double-delete) with your current code.

If you do not explicitly define a copy constructor and operator=, then the compiler will generate those for you. In this case, however, the default versions are not doing what they would need to do because you are managing memory manually within the class. As a general rule of thumb, if you ever define a destructor, then you probably need to define the copy constructor and operator= as well.

A simpler alternative which is just as good in some cases is to explicitly disable copying for the product type, but declaring the cctor and operator= as private (no need to define them).

Note, all of this is unnecessary with std::string because it handles its own copying internally, so the compiler-generated functions would do the right thing here.

void product::set_it(int prc ,char *nm  , int qlty , char *type )
{
	int i, o;
	this -> name = new char[strlen(nm)] ;

What about the previous memory pointed to by this->name? You have lost your handle to it but it's still "in use" as far as the program is concerned. This is called a memory leak.

This is a representative, but by no means complete list of the problems I see immediately.

[laserlight]

Im sorry but I expected this to be a peice of cake and now I refuse to let go of this way of doing it until I know all the things i did wrong and how to do them right. After that Ill probably rewrite the little code I have right now and use strings.

Besides what has already been mentioned, one of the things you did wrong is that your set_it member function overwrites the member pointers that already point to allocated memory, leading to memory leaks. Another problem is that you fail to appropriately handle exceptions: by right, you should have delete[]s in your exception handlers. This would be less troublesome if you were managing just one resource in this class, not two of them.

[leo235]

Problems:

without the name1 = NULL; at the end of the loop I get an error about a failed assertion.

when it comes to delete the debugger gives me weird errors that my debug assertion failed (Expression: _BLOCK_TYPE_IS_VALID(pHead -> nblockUse)



@Lindley:
T H A N K Y O U ! ! !

Code:

void product::set_it(int prc ,char *nm , int qlty , char *type )
{
int i, o;
this -> name = new char[strlen(nm)] ;

What about the previous memory pointed to by this->name? You have lost your handle to it but it's still "in use" as far as the program is concerned. This is called a memory leak.

that sounds like it could be my problem:



can i just

delete[] this ->name;
this -> name = new char[(strlen(nm)+1)] ;

?

name1 = NULL;
delete [] name1;

This won't do anything useful. Set it to NULL after deleting the pointer if you want. Although as I said, if you just put this array on the stack there would be no need for a delete.

I got an error about a failed assert without the name1 = NULL. I see now that delete doesnt do anything with NULL-pointers That beeing confusing enough (to me)
the fact that I didnt have to do the same with name2 and name3 is even less understandable.

why are you using dynamic memory here at all

because Im stubborn in the sense that Im actually just stupid. Although this is the very first bit of code Im writing in my "Im learning c++" project and might very well change substanzially once I got it working a bit.

Don't put "using" statements in header files, at least at unrestricted scope.

but then I would have to write std:: a million times wouldnt I? Also why shouldnt I?






My debugger says about the pointer name1 stuff like:
0x006b70 "gold"
112 "g" /*(not really that number) */
and cout << &name1 gives yet another address like 0x064h20

Am I completely mistaken in my belief that I can use that pointer to pass "gold" to an instance of a prodduct (p[0]) and and then put "silver" in the pointer to pass that to the next instance (p[1]) on the next run through the loop?


thanks a lot for your patience! http://www.codeguru.com/forum/images/icons/icon9.gif

[Lindley]

can i just

delete[] this ->name;
this -> name = new char[(strlen(nm)+1)] ;

?

Sort of. That is a good enough solution for 99% of cases. However, as was mentioned a few posts ago, it is not exception-safe. This isn't something you should concern yourself with too much until you get the hang of the basics more, but I'll explain anyway for future reference.

If you attempt to allocate a new char array and the "new" fails because you are out of memory, an exception will be thrown. If you don't catch exceptions anywhere, this effectively terminates your program. If you do catch exceptions, however, then it would be nice for the product object to still be in a valid state after the exception is thrown.

In the above code, you have already deleted the old pointer by the time you call new, so after the exception propagates your name pointer will be invalid. If you only had one pointer in the class, a simple solution would be to allocate first to a temporary pointer, then delete the old memory, then just copy the pointer over to name.

Since you have two pointers to work with, things get slightly more complicated----if the second "new" fails, you still have to release the memory from the first one. (The new that threw an exception did not, by definition, allocate anything.)

Like I said this is an advanced subject, so don't concern yourself with it too much. The above is an acceptable approach for now.

I got an error about a failed assert without the name1 = NULL. I see now that delete doesnt do anything with NULL-pointers That beeing confusing enough (to me)
the fact that I didnt have to do the same with name2 and name3 is even less understandable.

Presumably some part of your code prior to the delete managed to corrupt the heap around name1's memory block. This is only detected upon deletion, but happened sometime earlier, probably via an out-of-bounds array write.

but then I would have to write std:: a million times wouldnt I? Also why shouldnt I?

As I mentioned, usually you would put most of the function implementations which are presently in product.h into a product.cpp file. It's fine to put a using statement there. The reason you don't put using statements in header files is that it defeats the purpose of having namespaces in the first place.

A good example is a "vector" class. There's one in the standard library, std::vector<T>, which represents a dynamic array of T objects. But the word "vector" is also a mathematical term, so some math library might define a "vector" class as well.

So you have

#include <vector> // std::vector
#include "MyMath.h"

int main()
{
    std::vector<double> vals;
    mymath::vector x(4,3);
}

If <vector> (or ANY other header you include) had a "using namespace std;" statement in it, while simultaneously MyMath.h had a "using namespace math;" statement, then the above would be impossible because a compiler error would result at having two different things called "vector" brought into the active namespace. In order to keep the two libraries playing nicely with each other, you have to explicitly specify at least one of the namespaces (both is better for clarity in this case).

My debugger says about the pointer name1 stuff like:
0x006b70 "gold"
112 "g" /*(not really that number) */
and cout << &name1 gives yet another address like 0x064h20

Since name1 is a pointer, it's perfect normal for the address it holds as a value (0x006b70) to be different than the pointer's own address (0x064h20). Remember, a pointer is a variable just like any other---it has an address and a value. It just so happens that a pointer's value happens to be another address.

Note that if you defined name1 as an array on the stack, then its address and the address of the first element in the array would be the same.

Am I completely mistaken in my belief that I can use that pointer to pass "gold" to an instance of a prodduct (p[0]) and and then put "silver" in the pointer to pass that to the next instance (p[1]) on the next run through the loop?

Like most variables, pointers can be reassigned and used for a completely different purpose later. (The only exceptions are references and const variables.) That said, keep in mind that there is a difference between pointing name1 to a string "silver" instead, versus assigning the string "silver" into the memory pointed to by name1. In the latter case, you'll need to make sure there's enough memory available for the new string first. Also in the latter case, it will only do what you expect if the product type is saving a copy of the string rather than the pointer itself. In this case it is, so it's fine.

[Paul McKenzie]

@laserlight:
Im sorry but I expected this to be a peice of cake

So who fooled you into thinking that handling dynamic memory was easy or easier than just using a string class? It is not a "piece of cake", otherwise there would be no tools such as BoundsChecker, Purify, valgrind, or on the programming side, classes such as std::string and smart pointers.

Also, I agree with laserlight -- write a string class if you really want to learn how to use dynamically allocated memory properly. Otherwise you're just patching code here and there with new[] and delete[], with no coherent structure (leading to crashes, memory leaks, buffer overruns, etc.)

Regards,

Paul McKenzie

[Kaycay]

Why ? What is wrong with using STL string class ? it is not old pure C string, it is C++ more modern and stylish. if you write a string calss on your own, you will be back to use of old C string and memory handle issues.

-Kay

[Alterah]

What they are saying is instead of doing this dynamic memory allocation throughout the project, make a string class instead. This way, the OP can learn about dynamic memory allocation while writing something that can be reused later on. Ultimately, it would simply be for learning.

[leo235]

@Lindley
I am stunned by your ability to explain these things to me. For your willingness to do so the english language lacks an adequate expression of gratitude.

Seeing you now as the morally and intellectually superior beeing that you are I feel bad to continue asking you these questions but:

Since you have two pointers to work with, things get slightly more complicated----if the second "new" fails, you still have to release the memory from the first one. (The new that threw an exception did not, by definition, allocate anything.)

So I would simply need two temporary pointers?

please feel free not to answer this question as I will in fact use my filthy peasant-solution regardless. A filthy peasant-solution that I wouldnt have if it wasnt for you.

Like most variables, pointers can be reassigned and used for a completely different purpose later. (The only exceptions are references and const variables.) That said, keep in mind that there is a difference between pointing name1 to a string "silver" instead, versus assigning the string "silver" into the memory pointed to by name1. In the latter case, you'll need to make sure there's enough memory available for the new string first. Also in the latter case, it will only do what you expect if the product type is saving a copy of the string rather than the pointer itself. In this case it is, so it's fine.

In the latter case, you'll need to make sure there's enough memory available for the new string first

but it will overwrite that memory right? if I allocated 6 chars for "silver" and then put "gold" in it Im good and dont have to use delete and new or do i?

Also in the latter case, it will only do what you expect if the product type is saving a copy of the string rather than the pointer itself. In this case it is, so it's fine

it is because of your strcpy correct?







@all the others
You are probably right
I am definitely wrong
I will still try to learn all I can from this first program I wrote for I am the evil that must exist to balance the good that came into the world with Lindley

[laserlight]

So I would simply need two temporary pointers?

I am not sure what you mean by "temporary", so I will just say that you need two pointers. You do not need any additional pointers for those two strings.

but it will overwrite that memory right? if I allocated 6 chars for "silver" and then put "gold" in it Im good and dont have to use delete and new or do i?

You can reuse the already allocated space, but make sure that you don't end up with "golder" instead of just "gold".

Here's another reason why I suggest that you pause this and work on your own string class for practice: when you are concentrating on writing this string class, you only need to do this kind of thing once. Trying to do this in this class means either duplication of work for each string, or you need to write helper functions that would have been better off in a separate class anyway.

I will still try to learn all I can from this first program I wrote for I am the evil that must exist to balance the good that came into the world with Lindley

You're just being stubborn
This is not necessarily a bad trait, but you would be better off using your perseverance to first write your own string class for practice, and then use that (or std::string) in this product class. In fact, notice that most of the discussion of this thread has been about managing dynamically allocated strings, not about the design and implementation of a product class. Where the discussion has been about the product class, it was in the context of dynamically allocated strings.

[Paul McKenzie]

I will still try to learn all I can from this first program I wrote for I am the evil that must exist to balance the good that came into the world with Lindley

Yes, but take laserlights adivce.

Write the string class first. Then go back to your program, and use that string class instead of the new[]/deletes[] strewn all over the place. Not only will you learn to use dynamically allocated properly by writing the string class, you will also go back to your program that now exists, and plug in the string class usage instead of the new/delete twist and turns you're doing now.

Even if you did get your program to work without a string class and use new/delete as you're doing now, to be honest, the program will still be a poorly put together C++ program. We're trying to get you to not waste your time learning how to put together a poor C++ program, instead at the very least (since you want to learn how to handle memory) something that is more coherent (of course for learning purposes -- you should go to std::string as soon as you get your class working).

Regards,

Paul McKenzie