dynmaic memory allocation

[leo235]

OK so this program here:

int b;
	char *a= new char[40];
	cin >> a;
	cout << a;
		delete  a;
		a= new char[40];
		cin >> a;
	cout << a;
	cin >>b;

works wonderfully

however my real programm:

int prod = 0;
	int a = 0,b = 0,c = 0,d = 0,e = 0,f = 0;
	    char *name1 = new char[40] ;
	    char *name2 = new char [40];
	    char *name3 = new char [40];
	 
	product *p = new product [];

	for (int i=0; i != 1; )
	{
		    
		name1 = new char[40];
		name2 = new char[40];
		name3 = new char[40];
		
		
		cin >> name1;
		cout << endl;
		if (name1 = "p")
		{
			
			cin >> a;
			
			cin >> name2;
			
			cin>> b;
			
			cin >> name3;
			p[prod].set_it (a,name2,b,name3);
			
			cin >> i;
			p[prod].printprice();
			prod++;
			
			
			
			
			delete [] name1;
			//name1 = NULL;
			
			delete [] name2;
			//name2 = NULL;

			delete [] name3;
			//name3 = NULL;
		}
		else 
		{
			cout << "no"  << endl;
			return 7;
		}
	}

	cin >> a;
	return 0;

does not work at all.

and that when it comes to delete the debugger gives me weird errors that my debug assertion failed (Expression: _BLOCK_TYPE_IS_VALID(pHead -> nblockUse). if i set it up diffrently the error is that im trying to write after the heap buffer.

any kind of help would be great and i know that you guys will want me to use strings, but Im a little retarded.












YOU PROBABLY CAN DISREGARD

#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;
	this -> type = new char;
	this -> price = prc;
	this -> name = nm;
	this ->quality = qlty;
	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  ;
	this -> type = new char;
	this -> price = prc;
	this -> name = nm;
	this ->quality = qlty;
	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;
};

[webJose]

your set_it() is wrong. You allocate a single char, but you could have up to 40 chars comming in. You also think that assigning "nm" to this->name will copy the string into the new buffer you create a few lines above, when in fact you are just discarding the buffer (and creating a memory leak) and then assigning the nm pointer value (and therefore a pointer to the 40-char buffer you created in main()) to this->name. When the destructor of product is called, you try to delete a buffer that has already been deleted in main().

NOTE: I am calling main() the piece of code you show before the product class.

In main() in line 19, you write if (name = "p"), which is most likely an error. If you want that to read "if name1's contents equal the single letter p", then you would have to use 2 equal signs, but that for basic data types only. If you want to compare strings you need to use strcmp(), stricmp(), strncmp() or strnicmp(). If you were to write if (name == "p") you would be comparing pointer values, not the contents in the buffer pointed to by those pointers.

And that's what I see for now.

[Disch]

product *p = new product []; <- this is nonsense. How many products are you allocating? You need to specify.

I'm surprised that was even compiling.

Furthermore... you're just creating headaches for yourself you doing this much dynamic allocation. It really isn't necessary.

The easy way is to just use strings instead of char arrays. But even if you're a masochist and want to stick with char arrays for whatever reason, 40 chars is small enough to put on the stack. There's no need to use new[] for that.

[zb0S216C]

leo235 wrote:

            char *name1 = new char[40] ;
	    char *name2 = new char [40];
	    char *name3 = new char [40];
	 
	product *p = new product [];

	for (int i=0; i != 1; )
	{
		    
		name1 = new char[40];
		name2 = new char[40];
		name3 = new char[40];

(sic)

This is a serious issue and needs addressing. For one, name1, name2 and name3 already allocate 40 bytes of memory each before entering the loop. When the loop is entered, the memory previously allocated is discarded (without proper deletion) and each are assigned another 40 bytes. This needs fixing.

C++ doesn't support garbage collection, which means your program is leaking like a sieve.

_Wazzak

[webJose]

Ah, I missed that. That's actually not too bad considering the assignment of pointer values inside product::set_it(). The allocation outside the loop is just wrong though. That must go.

But allocation inside the loop could work if the pointers are inherited by the product object (as they are right now) and the memory is not deleted, which IS being deleted right now.

[zb0S216C]

I'll also add this to my previous post:

name1, name2 and name3 are only deleted if name1 contains the string "p". Either way, the memory MUST be released.

_Wazzak

[webJose]

name1, name2 and name3 are only deleted if name1 contains the string "p". Either way, the memory MUST be released.

Not true. Note how the comparison is really not a comparison because it has just one equal sign. Furthermore, in order to determine if name1 contains the string "p", it must use strcmp() or one of the other three functions I already mentioned.

[zb0S216C]

webJose wrote:

Note how the comparison is really not a comparison because it has just one equal sign. (sic)

It's highly possible that he* meant to use the equality operator (==), rather than the assignment operator. If this is truly the case, my point stands.

webJose wrote:

Furthermore, in order to determine if name1 contains the string "p", it must use strcmp() or one of the other three functions I already mentioned. (sic)

I agree with you on this, but the method of comparison was beyond the scope of my post. I was in fact questioning the logic of his* control paths and how they affect the allocated memory.

*He should be interpreted as he/she. *His should also be interpreted as his/her.

_Wazzak

[leo235]

your set_it() is wrong. You allocate a single char, but you could have up to 40 chars comming in. You also think that assigning "nm" to this->name will copy the string into the new buffer you create a few lines above, when in fact you are just discarding the buffer (and creating a memory leak) and then assigning the nm pointer value (and therefore a pointer to the 40-char buffer you created in main()) to this->name. When the destructor of product is called, you try to delete a buffer that has already been deleted in main().

but it works...
printprice gives me just what i want and the debugger shows that p[0] is kinda what I want it to be.
It just breaks down when it comes to the deletes in main

the second part sounds like its exactly my problem but i dont understand it. could you write me some code or comments to elaborate or shpow me how to get it right?
Also i swear the program worked two days ago when it was very similar to this, but I lost it.


Also generally i dont understand how my debugger tells me for example that name1 was
0x0034b70 "gold" (old is just the content of the cells following the one pointed to?)
112 (not really) "g" (why is this just one letter even when i allocated 40 chars?)
and cout << &name1 gives me yet another address (like 0x0050h43)

what does
char *name1 = new char[];
do exactly because I thought that would give me as many cells as needed.


Also many thanks to all of you who have hereby signed away your souls and agreed to make teaching me c++ your lifes quest. very gentlemanly of you.

[leo235]

AGAIN MASSIVE THANKS
OK I changed the code to:

#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 [4];

	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;
		if (name1 = "p")
		{
			cin >> a;
			cin >> name2;
			cin>> b;
			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;
		}
		else 
		{
			cout << "no"  << endl;
			return 7;
		}
	}

	//p[prod].printprice();
	//p[--prod].printprice();

	//p[1].set_it (100,"gold", 40, "Ressource");
	//p[1].printprice();

                                                                 


	//GEHT NICHT 
	//p1.~product () ;
	//p1.printprice();
	//product p1 (99, "gold");

	
	
	
	cin >> a;
	return 0;


}

and allocated 40 chars instead of one in the constructor of product and set_it
now the deletes are no problem anymore (even though i just worked on the one for name1)
but p[0].printptice() and p[1].printprice() give the same output...

[webJose]

char *name1 = new char[];

That is just plain wrong and it shouldn't compile. You need to specify the amount of chars you want inside the square brackets.

If you want single characters returned by cin, then why are you allocating 40 characters? A single char suffices. And on that note, you need not allocate it in the heap. Just declare the variable of type char in the stack.

If you continue to use strings, then product::set_it() needs to be something like:

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;
        strcpy(this -> name, nm);
	this ->quality = qlty;
        strcpy(this -> type, type);
	this->number_of_products++;
	for (i=0;i < number_of_products; i++)
	{
		o = 1;
	}
};

That corrects product::set_it(). You also need to correct product::~product() because you call delete on name and type, but you really need to call delete[].

If you use the above version of product::set_it(), then you must not allocate memory inside the for loop in main(), and instead you must allocate it outside of the loop. Since you will be allocating it outside the loop, you must also delete[] it outside the loop.

[leo235]

i made the changes to set_it you gave me webJose and the program behaves the same as before I still cant input multiple products as p[prod].printprice() and p[(prod-1)].printprice() still give the exact same output.
I see how your code is much better however.
I realize now that when i wrote
this->name = nm
before I was making the pointers p[whatever].name and name2 identical and probably deleted both when I was trying to delete just one right?
But why do I need to change the memory allocation inside the loop now? I mean i realize its kind of senseless but Im not wasting any ressources this way or writing any broken code am I?

Also my original question still stands
why did I get an error with delete [] name1
but

name1 = NULL;
			delete [] name1;

works just fine.


Oh and cant I do anything that makes it possible to allocate space for a new product whenever the user says he wants to make a new one?
or do i have to guess how many its going to be and write
product *p = new product [that_many];
?

[webJose]

The delete[] operator checks the given pointer. If the pointer is NULL, then it does nothing. This is why it throws an error if you try to delete a non-null pointer that has already been deleted.

You also need to post updated code because at this point we don't know how it looks like.

[leo235]

updated code:

// second.cpp : Defines the entry point for the console application.
//

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

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


int _tmain(int argc, char**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; //must be longer than 40 chars so youll need some mecahnism to make sure of that
		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;


}

#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)+1)];
	this -> type = new char[(strlen(type)+1)];
	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;
	delete[] this -> name;
	this -> name = new char[(strlen(nm)+1)] ;
	delete[] this -> type;
	this -> type = new char[(strlen(type)+1)];
	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;
};

updated problems:

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

[zb0S216C]

leo235 wrote:

failed asses. (sic)

Lol. I'd rephrase this.

Why're you setting name1 to null before deleting the memory associated with it?

_Wazzak

[leo235]

Lol. I'd rephrase this.

Why're you setting name1 to null before deleting the memory associated with it?

^^

Because without it:

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

the delete is senseless as it is ( as I learned recently) but i dont just wanna set name1 to NULL i want to delete it. just I cant.

[zb0S216C]

leo235 wrote:

char *name1 = new char[40] ;

...

name1 = NULL;
delete [] name1;

(sic)

This doesn't make sense. What you're doing here is allocating 40 bytes + 4 bytes. After that, you're forcing name1 to forget the allocated region of memory by setting name1 to null. Then, you delete a null pointer (it won't have any effect anyway). The assignment statement and the delete statement need to be switched around. Note that setting a pointer (a pointer that points to an allocated block) to null doesn't delete the memory.

I'll add a side note here. A pointer can only point to one location at a time, right? Well, I've thought of a little trick which allows you to allocate n arrays and keep track of them all. Here's an example:

char *Region( new( std::nothrow ) char[3] );
int *Address( ( int * )Region );

Region = new( std::nothrow ) char[3];

// Delete the second array.
delete [] Region;

// Delete the first array.
Region = ( char * )Address;
delete [] Region;

Region = Address = nullptr;

I'm not saying you should use it, but it might come in handy, like it did for me :)

_Wazzak

[leo235]

The assignment statement and the delete statement need to be switched around.

when i do that I get the assertion failure







char *Region( new( std::nothrow ) char[3] );

is that the same as

char *Region = new std::nothrow char[3] ;
?

If not what do the first 2 lines do?

[zb0S216C]

leo235 wrote:

when i do that I get the assertion failure (sic)

I think it's time for a redesign of your program.

leo235 wrote:

char *Region( new( std::nothrow ) char[3] );

is that the same as

char *Region = new std::nothrow char[3] ;
? (sic)

Yes, they both invoke char's constructor. However, omitting the parentheses around std::nothrow is an syntax error. They need to be there.

_Wazzak