Function returns wrong value

[hazarada]

ok let me set up a sample of this problem:

data structure: (simplified version)

struct a {
int value;
   a::a(int val) {
      value = val;
   }
   a* clone() {
      a* ret = new a(value);
      return ret;
   }
};

data instance: (simplified version)

struct aInst {
   a* b;
   a* c;
   aInst::aInst() {}
   aInst::aInst(a* base) {
      b = base;
      c = b->clone();
   }

}

data collection: (simplified version)

struct collection {
   aInst* coll;
   int count;
   collection::collection(int size) {
      coll = new aInst[size];
      count = 0;
   }
   add(a* base) {
      coll[count] = aInst(base);
   }
}

There, and here is whats happening: if i do this

main {
   a* foo = new a(10);
   a* bar = a.clone();
}

then everything works perfectly, however if i do this:

main {
   a* foo = new a(10);
   collection stuff (5);
   stuff.add(foo);
   cout << stuff.coll[0].b->value << " " << stuff.coll[0].c->value;
}

i get this as output: "10 4277075694"
The reason is that when i call the a::clone() from inside the instance structure it successfully creates the new a, returns the pointer to ret but then the function returns some random gibberish pointer and not ret. Wth is going on here? Thank you.

[guestgulkan]

I get 10 10 (both in visual studio and mingw-gcc) - maybe there is something else in your real code that is causing the issue.

have you written your own copy constructor and assignment operator functions (which you would need to do in the real code) - maybe there is an issue there.

[ne555]

¿what the hell are you trying to do? It's seem to be unnecessary complicated.
Try to provide a minimal example that reproduces your issue.

So many news, I guess that you've got some destructors. I'm betting to heap corruption (wrong assignment operator)

[hazarada]

here is the actual code

struct bVertex { //resides in bMesh
	D3DXVECTOR3 pos;
	D3DXVECTOR4 color;
	D3DXVECTOR2 texCoord;
	bVertex::bVertex() {}
	bVertex::bVertex(float x, float y, float z, float r, float g, float b, float a, float u, float v) {
		pos = D3DXVECTOR3(x, y, z);
		color = D3DXVECTOR4(r, g, b, a);
		texCoord = D3DXVECTOR2(u, v);
	}
};
struct bMesh { //resides in heap
	bVertex* Vertices;
	unsigned int* Indices;
	unsigned int CountVertex;
	unsigned int CountIndex;
	unsigned int MaxVertices;
	unsigned int MaxIndices;
	bMesh::bMesh(unsigned int VertexCount, unsigned int IndexCount) {
		Vertices = new bVertex[VertexCount];
		Indices = new unsigned int[IndexCount];
		MaxVertices = VertexCount;
		MaxIndices = IndexCount;
		CountVertex = 0;
		CountIndex = 0;
	}
	bMesh::~bMesh() {
		delete[] Vertices;
		delete[] Indices;
	}
	bMesh* Clone() {
		bMesh* ret = new bMesh(MaxVertices, MaxIndices);
		ret->CountIndex = CountIndex;
		ret->CountVertex = CountVertex;
		ret->Vertices = new bVertex[MaxVertices];
		ret->Indices = new unsigned int[MaxIndices];
		memcpy(ret->Vertices, Vertices, sizeof(bVertex) * CountVertex);
		memcpy(ret->Indices, Indices, sizeof(unsigned int) * CountIndex);
		return ret;
	}
	void AddVertex(bVertex vertex) {
		if (CountVertex == MaxVertices) return;
		Vertices[CountVertex] = vertex;
		CountVertex++;
	}
	void AddIndex(unsigned int index) {
		if (CountIndex == MaxIndices) return;
		Indices[CountIndex] = index;
		CountIndex++;
	}
};

struct bMeshInstance { //resides in bCollection
	bMesh* pIn;
	bMesh* pOut;
	bMeshInstance::bMeshInstance() {}
	bMeshInstance::bMeshInstance(bMesh* base) {
		pIn = base;
		pOut = pIn->Clone();
	}
	bMeshInstance::~bMeshInstance() {
		delete pOut;
	}
};

struct bCollection {
	bMeshInstance* Collection;
	unsigned int MaxCollection;
	unsigned int CountCollection;
	unsigned int CountIndex;
	
	bCollection::bCollection(unsigned int size) {
		Collection = new bMeshInstance[size];
		MaxCollection = size;
		CountCollection = 0;
		CountIndex = 0;
	}
	bCollection::~bCollection() {
		delete[] Collection;
	}
	void AddInstance(bMesh* pMesh) {
		if (MaxCollection == CountCollection) return;
		Collection[CountCollection] = bMeshInstance(pMesh);
		CountCollection++;
		CountIndex += pMesh->CountIndex;
	}
	void* DumpVertices(void* dest) { //returns new start position for further dumps
		unsigned int Copied = 0;
		void* To = dest;
		for (int i = 0; i < CountCollection; i++) {
			memcpy(To, Collection[i].pOut->Vertices, Collection[i].pOut->CountVertex);
			Copied += Collection[i].pOut->CountVertex;
			To = (void*)((unsigned int)dest + Copied * sizeof(bVertex));
		}
		return To;
	}
	void* DumpIndices(void* dest) { //returns new start position for further dumps
		unsigned int Copied = 0;
		void* To = dest;
		for (int i = 0; i < CountCollection; i++) {
			memcpy(To, Collection[i].pOut->Indices, Collection[i].pOut->CountIndex);
			Copied += Collection[i].pOut->CountIndex;
			To = (void*)((unsigned int)dest + Copied * sizeof(unsigned int));
		}
		return To;
	}
};

[guestgulkan]

I reckon that this

struct bMeshInstance { //resides in bCollection
	bMesh* pIn;
	bMesh* pOut;
	bMeshInstance::bMeshInstance() {}
	bMeshInstance::bMeshInstance(bMesh* base) {
		pIn = base;
		pOut = pIn->Clone();
	}
	bMeshInstance::~bMeshInstance() {
		delete pOut;
	}
};

needs a proper (deep copy) copy and assignment operator functions

otherwise, this function

	void AddInstance(bMesh* pMesh) {
		if (MaxCollection == CountCollection) return;
		Collection[CountCollection] = bMeshInstance(pMesh); //** problems here if   bmeshInstance does not have a proper (deep copy) assignment operator?
		CountCollection++;
		CountIndex += pMesh->CountIndex;

[hazarada]

i dont understand, how would that help? the only part that needs deep copying is pOut (pIn should always remain the same across instances) and by creating a new bMesh in the clone should take care of that? the default assignment operator is just transferring over pointers which is exactly what i want

in case you misunderstood bCollection.bMeshInstance[x] works fine, its the bCollection.bMeshInstance[x].pOut that is broken

EDIT: i also tried creating a bmeshinstance inside main both in stack and heap and works fine there, the only place it doesn't work is when its called in bCollection struct. I also tried adding a bogus constant return to the clone function which oddly enough triggered an exception inside bMesh deconstructor, as far as i know during the sample execution no bMeshes should be destroyed so.. weird

further investigation - the line you pointed out triggers bMeshInstance deconstructor for some reason but only when executed inside bCollection, mind blown

Ok so if i understand correctly when calling
Collection[CountCollection] = bMeshInstance(pMesh);
first the right hand side gets evaluated and a bMeshInstance is created, then the bMeshInstance is copied over to the left side and then the original is destroyed? That really sucks because i cant just remove the deconstructor and leave all the pOut's hanging around after i delete instances. Nor do i like cloning over the pOut's again because thats just inefficient.

Is there any way to create the new bMeshInstance inside the Collection array to begin with? without resorting to converting the bMeshInstance array into a bMeshInstance pointer array that is.

EDIT: success! i realized i already had a bunch of bMeshInstances in the collection from the collection constructor so no need to renew them, made few changes

struct bMeshInstance { //resides in bCollection
	bMesh* pIn;
	bMesh* pOut;
	bMeshInstance::bMeshInstance() {pOut = NULL;}
	bMeshInstance::bMeshInstance(bMesh* base) {
		pIn = base;
		pOut = pIn->Clone();
	}
	bMeshInstance::~bMeshInstance() {
		MessageBox(0, L"hai", 0, 0);
		delete pOut;
	}
	void AssignMesh (bMesh* base) {
		delete pOut;
		pIn = base;
		pOut = pIn->Clone();
	}
};
struct bCollection {
	void AddInstance(bMesh* pMesh) {
		if (MaxCollection == CountCollection) return;
		Collection[CountCollection].AssignMesh(pMesh);
		CountCollection++;
		CountIndex += pMesh->CountIndex;
	}
};

working fine now

[ne555]

Rule of three: if you need a destructor, a copy constructor or an assignment operator, you need the three.

If you just shallow copy the pointers, then you will have a double delete. (pOut)

Again ¿what the hell are you trying to do? It's seem to be unnecessary complicated.