unsure about memcpy

I am not sure on the memcpy usage of my IntList here, is this correct... I don't think it is correct as when I try to add something to the list after a copy constructor it causes a segfault
#include <cstdio>
#include <cstring>
#include <cassert>

#include "IntList.h"

IntList::IntList()
: itsValueCount(0), itsCurrentAlloc(AllocChunk)
{
    itsValues = new int[AllocChunk];
}

IntList::~IntList()
{
    delete [] itsValues;
}

IntList& IntList::operator=(const IntList& list)
{
    if (this != &list) { // know myself
        delete [] itsValues;
        itsValues = new int[list.length()];
        memcpy(itsValues, list.itsValues, sizeof(int) * list.length());
        }
    return *this;
}

IntList::IntList(const IntList& list)
{
    itsValues = new int[list.length()];
    memcpy(itsValues, list.itsValues, sizeof(int) * list.length());
}


void IntList::add(int value)
{
    if (itsValueCount == itsCurrentAlloc) {
        //
        // No room...must expand
        itsCurrentAlloc = itsValueCount + AllocChunk;
        int *newblock = new int[itsCurrentAlloc];
        for (int i = 0; i < itsValueCount; i++)
            newblock[i] = itsValues[i];
        delete [] itsValues;
        itsValues = newblock;
        }
    itsValues[itsValueCount++] = value;
}

void IntList::print(const char *label) const
{
    printf(label, ""); // "" to suppress a spurious warning...
    const char *fmt = "%d";
    for (int i = 0; i < itsValueCount; i++) {
        printf(fmt, itsValues[i]);
        fmt = ", %d";
        }
    printf("\n");
}

int& IntList::operator[ ](int pos)
{
    assert(pos >= 0 && pos < itsValueCount);
    return itsValues[pos];
}

IntList operator*(int n, IntList& list){
	IntList r;
	for (int i = 0; i < list.length(); i++)
		r.add(list[i] * n);
	return r;
}

IntList operator*(IntList& list, int n){
	return operator*(n, list);
}


IntList operator+(IntList& lhs, IntList& rhs)
{
	IntList r;
	int temp = rhs.length();
	if (lhs.length() < rhs.length())
		temp = lhs.length();
	

	for (int i = 0; i < temp; i++)
		r.add(lhs[i] + rhs[i]);

	if (lhs.length() != rhs.length()){
		if (lhs.length() > rhs.length())
			for (int j = temp; j < lhs.length(); j++)
				r.add(lhs[j]); 
		else
			for (int j = temp; j < rhs.length(); j++)
				r.add(rhs[j]); 
	}

	return r;
}

Open in new window

kuntilanakAsked:
Who is Participating?
I wear a lot of hats...

"The solutions and answers provided on Experts Exchange have been extremely helpful to me over the last few years. I wear a lot of hats - Developer, Database Administrator, Help Desk, etc., so I know a lot of things but not a lot about one thing. Experts Exchange gives me answers from people who do know a lot about one thing, in a easy to use platform." -Todd S.

evilrixSenior Software Engineer (Avast)Commented:
You shouldn't use memcpy to copy classes like this, you should do a member by member copy. It's not safe.
0
kuntilanakAuthor Commented:
well if I do:

then I can't make the IntList& list in the parameter as a const?
IntList& IntList::operator=(IntList& list)
{
    if (this != &list) { // know myself
        delete [] itsValues;
        itsValues = new int[list.length()];
	 for (int i = 0; i < list.length(); i++)
		itsValues[i] = list[i];
    }
    return *this;
}

Open in new window

0
evilrixSenior Software Engineer (Avast)Commented:
Sorry, my mistake... you're copying a member of the class -- which I guess is an array of type int?

Looking...
0
Cloud Class® Course: Python 3 Fundamentals

This course will teach participants about installing and configuring Python, syntax, importing, statements, types, strings, booleans, files, lists, tuples, comprehensions, functions, and classes.

kuntilanakAuthor Commented:
yes! array type int
0
evilrixSenior Software Engineer (Avast)Commented:
Are you sure it's the cc_tor that's the issue? I can't see anything obviously wrong with it.
0
kuntilanakAuthor Commented:
try testing it with the following
// Value: 10 points

#include "IntList.h"

int main()
{
    IntList L1, L2;

    for (int i = 0; i < 20; i += 3)
        L1.add(i);

      
    L2 = L1;
      
    IntList L3(L2);

	
    L1.add(100);
		
    L2.add(200);
    L2.add(250);

    L3.add(300);
    L1.print("L1: ");
    L2.print("L2: ");
    L3.print("L3: ");
}

Open in new window

0
kuntilanakAuthor Commented:
here's the IntList.h


I just hate segfault errors
#ifndef _intlist_h_
#define _intlist_h_
class IntList {
    public:
        IntList();
        ~IntList();
	 IntList& operator=(const IntList& rhs);
	 IntList(const IntList& list);
        void add(int value);
        void print(const char *label) const;
        int length() const { return itsValueCount; }
	 int& operator[](int pos);
	 operator int() { // convert to int
    		int res = 0;
		for (int i = 0; i < itsValueCount; i++)
			res += itsValues[i]; 
		return res; 
 	 } 
    private:
        int *itsValues;
        int itsValueCount;
        int itsCurrentAlloc;
        static const int AllocChunk = 20;
};


    IntList operator+(IntList& lhs, IntList& rhs);
    IntList operator*(IntList& list, int n);
    IntList operator*(int n, IntList& list);
#endif

Open in new window

0
evilrixSenior Software Engineer (Avast)Commented:
I'll need the header code too
0
kuntilanakAuthor Commented:
it's above you
0
evilrixSenior Software Engineer (Avast)Commented:
Ah, you beat me to it :)

testing.
0
evilrixSenior Software Engineer (Avast)Commented:
Your copy constructor isn't copying all the members so when you next try to add with L3 itsValueCount has a crappy value and is causing memory allocation failure.
0
kuntilanakAuthor Commented:
hmmm....why isn't it copying everything, what's wrong with my copy constructor
0
evilrixSenior Software Engineer (Avast)Commented:
>> hmmm....why isn't it copying everything, what's wrong with my copy constructor
You've not coded it to copy everything, only the int array :)
0
kuntilanakAuthor Commented:
you mean my memcpy in the copy constructor doesn't copy the values?
0
kuntilanakAuthor Commented:
what should I do to fix it? That's my question.... as I still don't see what to fix further on in my copy constructor
0
evilrixSenior Software Engineer (Avast)Commented:
>> you mean my memcpy in the copy constructor doesn't copy the values?
No, I mean you've only written code to copy the array. What about the other member values?

Same in your assignment operator.

After it's fixed, though, you have a heap corruption -- you're probably double deleting something.
IntList::IntList(const IntList& list)
{
    itsValueCount = list.itsValueCount; // <----- This?
    itsCurrentAlloc = list.itsCurrentAlloc; // <----- This?

    itsValues = new int[list.length()];
    memcpy(itsValues, list.itsValues, sizeof(int) * list.length());
}

Open in new window

0

Experts Exchange Solution brought to you by

Your issues matter to us.

Facing a tech roadblock? Get the help and guidance you need from experienced professionals who care. Ask your question anytime, anywhere, with no hassle.

Start your 7-day free trial
kuntilanakAuthor Commented:
what do you mean a heap corruption? did you run it using valgrind? as I can run it fine after adding the values you suggested
0
evilrixSenior Software Engineer (Avast)Commented:
>> what do you mean a heap corruption?
I mean somehow you are corrupting the heap

>> did you run it using valgrind?
No, I am using the debug CRT with Visual Studio and that's reporting a heap corruptions -- it looks like something in your add function is amiss but I'm still trying to track is down.

>> as I can run it fine after adding the values you suggested
The heap corruption won't necessarily show up initially. I am seeing it because the debug CRT tracks things like this an reports them. Now doubt, valgrind will also report something is amiss.
0
kuntilanakAuthor Commented:
hmmm....that's weird... let me try to re-analyze the code once again and see what happens.... see which one of us finds it faster
0
evilrixSenior Software Engineer (Avast)Commented:
Sussed it.

The amount of memory to new is wrong in both the assignment operator and the cc_tor. You need to allocated list.itsCurrentAlloc and not list.length() because all your code assumes the array is allocated in chunks and if you only allocate length you only allocate enough to hold the length of the array being copied and not the actual size of the buffer in the class being copied.

IntList& IntList::operator=(const IntList& list)
{
	if (this != &list) { // know myself
		delete [] itsValues;
		itsValues = new int[list.itsCurrentAlloc];
		memcpy(itsValues, list.itsValues, sizeof(int) * list.length());
		itsValueCount = list.itsValueCount;
		itsCurrentAlloc = list.itsCurrentAlloc;
	}
	return *this;
}

IntList::IntList(const IntList& list)
{
    itsValueCount = list.itsValueCount;
    itsCurrentAlloc = list.itsCurrentAlloc;

	itsValues = new int[list.itsCurrentAlloc];
	memcpy(itsValues, list.itsValues, sizeof(int) * list.length());
}

Open in new window

0
It's more than this solution.Get answers and train to solve all your tech problems - anytime, anywhere.Try it for free Edge Out The Competitionfor your dream job with proven skills and certifications.Get started today Stand Outas the employee with proven skills.Start learning today for free Move Your Career Forwardwith certification training in the latest technologies.Start your trial today
C++

From novice to tech pro — start learning today.