?
Solved

unsure about memcpy

Posted on 2010-04-02
20
Medium Priority
?
303 Views
Last Modified: 2012-05-09
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

0
Comment
Question by:kuntilanak
  • 10
  • 10
20 Comments
 
LVL 40

Expert Comment

by:evilrix
ID: 29505327
You shouldn't use memcpy to copy classes like this, you should do a member by member copy. It's not safe.
0
 

Author Comment

by:kuntilanak
ID: 29505510
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
 
LVL 40

Expert Comment

by:evilrix
ID: 29505785
Sorry, my mistake... you're copying a member of the class -- which I guess is an array of type int?

Looking...
0
The new generation of project management tools

With monday.com’s project management tool, you can see what everyone on your team is working in a single glance. Its intuitive dashboards are customizable, so you can create systems that work for you.

 

Author Comment

by:kuntilanak
ID: 29505811
yes! array type int
0
 
LVL 40

Expert Comment

by:evilrix
ID: 29506138
Are you sure it's the cc_tor that's the issue? I can't see anything obviously wrong with it.
0
 

Author Comment

by:kuntilanak
ID: 29506168
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
 

Author Comment

by:kuntilanak
ID: 29506215
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
 
LVL 40

Expert Comment

by:evilrix
ID: 29506251
I'll need the header code too
0
 

Author Comment

by:kuntilanak
ID: 29506264
it's above you
0
 
LVL 40

Expert Comment

by:evilrix
ID: 29506272
Ah, you beat me to it :)

testing.
0
 
LVL 40

Expert Comment

by:evilrix
ID: 29506542
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
 

Author Comment

by:kuntilanak
ID: 29506585
hmmm....why isn't it copying everything, what's wrong with my copy constructor
0
 
LVL 40

Expert Comment

by:evilrix
ID: 29506753
>> 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
 

Author Comment

by:kuntilanak
ID: 29506798
you mean my memcpy in the copy constructor doesn't copy the values?
0
 

Author Comment

by:kuntilanak
ID: 29506901
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
 
LVL 40

Accepted Solution

by:
evilrix earned 2000 total points
ID: 29507036
>> 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
 

Author Comment

by:kuntilanak
ID: 29507282
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
 
LVL 40

Expert Comment

by:evilrix
ID: 29507492
>> 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
 

Author Comment

by:kuntilanak
ID: 29507551
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
 
LVL 40

Expert Comment

by:evilrix
ID: 29508588
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

Featured Post

Never miss a deadline with monday.com

The revolutionary project management tool is here!   Plan visually with a single glance and make sure your projects get done.

Question has a verified solution.

If you are experiencing a similar issue, please ask a related question

Basic understanding on "OO- Object Orientation" is needed for designing a logical solution to solve a problem. Basic OOAD is a prerequisite for a coder to ensure that they follow the basic design of OO. This would help developers to understand the b…
Examines three attack vectors, specifically, the different types of malware used in malicious attacks, web application attacks, and finally, network based attacks.  Concludes by examining the means of securing and protecting critical systems and inf…
Video by: Grant
The goal of this video is to provide viewers with basic examples to understand and use for-loops in the C programming language.
The viewer will learn how to pass data into a function in C++. This is one step further in using functions. Instead of only printing text onto the console, the function will be able to perform calculations with argumentents given by the user.

593 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question