2

I have an overloaded = operator which is correctly passing some of the values in the particle class, but not other values. I have troubleshooted online, and could not find anything which directly pertained to this problem. My peer who is fairly competent in C++ could not assist, and recommended I post here. Any assistance would be greatly appreciated.

All the values for width, height, wrap, particlen and array were passing perfectly. However, the values for xpos, ypos, xvel, and yvel were passing with incorrect values. For xpos and ypos, every 11th element was passing correctly, but all the other elements were equal to zero; I expect none of these values to be zero. In main, in the operation immediately proceeding gen1 = gen0;, all the dereferenced values for gen0 were correct. I estimate that they were somehow not passed correctly? I will happily post more code/info if required. Again, any insights would be deeply appreciated. Thank you.

Relevant code:

class particle{             
private:
    int* xpos;  
    int* ypos; 
    int* xvel;
    int* yvel;
    int* array;             
    int width;
    int height; 
    int wrap;
    int particlen;
public:
    void operator=(const particle&);
}

Relevant parts of overloaded = operator:

void particle::operator=(const particle &current){
    int a,b,i,j;
    width = current.width;
    height = current.height;
    wrap = current.wrap;
    particlen = current.particlen;
    array = new int[width*height];
    xpos = new int[particlen];
    ypos = new int[particlen];
    xvel = new int[particlen];
    yvel = new int[particlen];

    for(a=0; a<height; a++){
        for(b=0; b< width; b++){
        this->array[a*width + b] = current.array[a*width + b];
        }
    } 

for(i = 0; i < particlen; i++){
    this->xpos[i] = current.xpos[i];
    this->ypos[i] = current.ypos[i];
    this->xvel[i] = current.xvel[i];
    this->yvel[i] = current.yvel[i];
}

Relevant parts of main:

int main(){
    particle gen0,gen1;
    gen1 = gen0;
}

With Sehe's suggestion, I edited my code. However, I am now getting memory allocation problems with the most simple of commands. The << operator, the first operation called in my main does not allow my array variable to be set equal to an integer in a read in file. I definitely know that the file is readable, as the first output line prints. However, I then get a segmentation fault 11 at array[i*width + j] = k;. GDB prints the output: program received signal EXC_BAD_ACCESS, Could not access memory. Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000000 0x000000010001c4f4 in particle::operator<< (this=0x7fff5fbfcb08, file=0x7fff5fbffa90 "100100") at Before typedef 1 vector.cpp:686 686 array[i*width + j] = k;

I have looked up the correct terminology for the vector library online, and thought my syntax was correct. Any ideas?

Update (9:00 EST 2nd Aug)

I am still having trouble with a NULL pointer. When using gdb to debug, I receive the message:

KERN_INVALID_ADDRESS at address: 0x0000000000000000 0x000000010001d807 in particle::operator<< (this=0x7fff5fbfcb08, file=0x7fff5fbffa90 "5040") at Before typedef 1 vector.cpp:688 688 array[(i*width + j)] = k;

I have little experience with NULL pointers; any suggestions as to why this is happening, and how to fix it? Here is the relevant code (the total code is about 900 lines)

class particle{             
private:
    std::vector<int> xpos;  
    std::vector<int> ypos; 
    std::vector<int> xvel; 
    std::vector<int> yvel; 
    std::vector<int> array; 
    int width;      
    int height;     
    int wrap;       
    int particlen; // number of particles read in
public:
    void operator<<(particle);
    void Collision(int, int, particle); 
    void operator>>(char*);
    void operator<<(char*);
};

void particle::operator<<(char* file) // Reads initial input file
{
ifstream in_file;
in_file.open( file);    // open the file
int i,j,k;
}
in_file >> width >> height >> wrap >> particlen;
    for(i=0; i<height; i++){
        for(j=0; j< width; j++){
            in_file >> k;
            array[i*width + j] = k; // This is line 688 which GDB references
        }
    }
}

void particle::operator<<(particle current){
int i,k,j,l;

for(k = 0; k < height*width; k++)
{
    array[k] = 0;
}

k = 0;

for(i=0; i < particlen; i++)
{
    cout << "Current x pos is" << current.xpos[i] << endl;
}

for(i=0; i < particlen; i++)
{

    if(array[ (current.ypos[i]-1)*width + (current.xpos[i] - 1) ] == 0 )
    {
    //cout << "Current X position[" << i << "] is" << current.xpos[i] << endl;
        xpos[i] = current.xpos[i] + (current.xvel[i])*timeinc;
        if(xpos[i] > width) xpos[i] = xpos[i] - width; 
        ypos[i] = current.ypos[i] + (current.yvel[i])*timeinc;
        if(ypos[i] > width) ypos[i] = ypos[i] - height;

    //cout << "Next X position[" << i << "] is" << xpos[i] << endl;
    }

    if(array[ (current.ypos[i]-1)*width + (current.xpos[i] - 1) ] > 1 && current.JC[i] != 1)
    {
        for(l=i+1; l < particlen; l++)
        {
            if(current.xpos[l] == current.xpos[i] && current.ypos[l] == current.ypos[i] && current.JC[l] != 1)
            {
            Collision(i,l,current);
            }
        }
    }
}

for(i=0; i < particlen; i++)
{
    array[ (xpos[i]-1)*width + (ypos[i]-1) ] = 1;
}
Display();}

int main()
{
    char filename[256];
    particle gen0, gen1;
    gen0 << filename;
    gen1 = gen0;
    gen0 << gen1; // Calculates the next state of gen0, based on the values in gen1.
}

I know this is long, but I thought that this is the minimal information that I could provide. I thought that perhaps the problem is with the gen0 << gen1 operator, so I included the method.

user2638374
  • 37
  • 1
  • 7
  • This is a guaranteed memory leak; you're not deleting the original arrays. – Oliver Charlesworth Aug 02 '13 at 00:07
  • @OliCharlesworth I suspect the reverse: I suspect missing copy constructors shallow copying the pointers, but they get freed in one instance. – sehe Aug 02 '13 at 00:08
  • 1
    Use `std::vector` for your dynamic arrays instead of pointers and `new`. You are just asking for trouble. – Casey Aug 02 '13 at 00:10
  • @OliCharlesworth Sorry, I am not completely clear how that would help. Could you please explain? Thank you. – user2638374 Aug 02 '13 at 00:11
  • @user2638374 You edit **still** doesn't show any constructor resizing the vectors (or the original arrays, for that matter). I can only assume that you never had that in the first place. Please, post a [SSCCE](http://sscce.org/) ***minimal but complete*** – sehe Aug 02 '13 at 13:34

1 Answers1

2

Like I hinted in the comment, 99% says you're violating Rule Of Three

Notably, you have the assignment operator, but there's no copy constructor in sight.. This could mean that if copied, the array pointers will end up "illegally" shared. I also assume you have a destructor somewhere that deletes those arrays (in one instance), thus invalidating the arrays that were copied into another instance.

Fix it by avoiding manual memory management in the first place:

#include <vector>

class particle{             
    std::vector<int> xpos;  
    std::vector<int> ypos; 
    std::vector<int> xvel;
    std::vector<int> yvel;
    std::vector<int> array;             
    int width;
    int height; 
    int wrap;
    int particlen; // or use `xpos.size()` e.g.
};

int main()
{
    particle gen0,gen1;
    gen1 = gen0;
}

PS. You might want to consider using v.at(i) instead of v[i] whenever you want bounds-checking. This can be considerable help in debugging issues.

Community
  • 1
  • 1
sehe
  • 374,641
  • 47
  • 450
  • 633
  • Close: you forgot to delete the manual assignment operator and let the compiler handle it. Rule of Zero! – Casey Aug 02 '13 at 00:12
  • Wonderful; thank you for your assistance. I will implement this, and hopefully have it working. If I have any further questions I will let you know. – user2638374 Aug 02 '13 at 00:17
  • You need to implement the default constructor, otherwise your POD members are uninitialized. – Benjamin Lindley Aug 02 '13 at 00:23
  • @BenjaminLindley Thanks for the comment. I did implement a default constructor; just didn't include it in the posted code. – user2638374 Aug 02 '13 at 00:24
  • @sehe I'm still having trouble with memory allocation. For example, whenever I run the code: for(i=0; i> k; array[i*width + j] = k; } } I get a segmentation fault, and GDB gives me KERN_INVALID_ADDRESS at address: 0x0000000000000000. Is this possibly a problem with my compiler? – user2638374 Aug 02 '13 at 04:12
  • @user2638374 Look closely: it is a problem with a null pointer. This can't happen with std::vector. I'm assuming you might be allocating other objects (`particle` itself, maybe) dynamically too and you're invoking a method through a null-pointer? A debugger (or `valgrind`) will come in handy to find the mistake, if you cannot find it through code inspection. – sehe Aug 02 '13 at 08:02
  • @sehe GDB tells me that 0x000000010001d807 in particle::operator<< (this=0x7fff5fbfcb08, file=0x7fff5fbffa90 "5040") at Before typedef 1 vector.cpp:688 688 array[(i*width + j)] = k; This seems to suggest that the problem is with the std::vector? – user2638374 Aug 02 '13 at 12:49
  • @user2638374 Wait, what? You're not making any sense. If you post a minimal sample I'll be glad to look at it. But your mess of a comment doesn't in the least indicate a problem with std::vector. – sehe Aug 02 '13 at 12:53
  • @sehe Apologies for the abrupt reply; I have updated my example with a more extensive example of my code. – user2638374 Aug 02 '13 at 13:25
  • @user2638374 See my edit. You haven't adjusted the constructors (which you didn't show in the original code anyways) to resize the vectors. This is the place where you _used to have_ `new ...[particlen]` or (god forbid) `malloc` calls. – sehe Aug 02 '13 at 13:32