Can't assign an member variable address to a pointer

Question

Layer::Layer(int LayerSize, Layer PrevLayer){

Size=LayerSize;

Column=(Neuron*)malloc(Size);

for(int i=0;i<Size;i++){


    Column[i]=Neuron(LayerSize,LayerSize,LayerSize);  

    Input[i]=&Column[i].Input;                        //1

    PrevLayer.Output[i]=Input[i];                     //2

}

I'm trying to have Input[i] point to the input of the corresponding neuron. While the memory addresses seem correct, when I try to assign the address value of the member variable Input at line //1, the program crashes. Any idea what's wrong, or better methods that I could use?

Below are the classes with the relevant members

}

class Neuron{

public:

    Neuron(int PrevColumnSize, int ThisColumnSize, int NextColumnSize);

                            //Constructor: Generates a Neuron with random values

    double Input;           //input of each neuron


};


class Layer{

private:

    int Size;               //Number of Neurons in the layer

public:

    Layer(int LayerSize);   //Constructor; Layer with no attached layers; used at the start of a network


    Layer(int LayerSize, Layer PrevLayer);

                            //Constructor; Layer which attaches itself to the next, and the previous layers; unused

    Neuron* Column;         //Column of Neurons

    double** Input;         //Inputs to Neurons

};

Possible duplicate of [Is uninitialized local variable the fastest random number generator?](https://stackoverflow.com/questions/31739792/is-uninitialized-local-variable-the-fastest-random-number-generator) — LogicStuff, Jul 23 '17 at 20:29
Off topic: `Layer PrevLayer` is pass by value. Any changes you make to `PrevLayer` are made to a copy and will be lost at the end of this function when `PrevLayer` goes out of scope and is destroyed. This would be a really good time to familiarize yourself with [What is The Rule of Three?](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) — user4581301, Jul 23 '17 at 20:36
Thanks user4581301, I've also tried defining it as `layer* PrevLayer`, and passing it an address, but that didn't work either — lordflashhart, Jul 23 '17 at 20:39
Don't use `malloc` in C++. You'll avoid many bugs, like for example allocating X bytes when you want X array elements. — molbdnilo, Jul 23 '17 at 20:40
That's why I call it off topic. That's the *next* two bugs you're going to run into. Your current bug is trivial. Probably why no one's come out and told you yet. Play with it a bit longer, maybe make a [mcve] and you'll see it yourself. — user4581301, Jul 23 '17 at 20:42
Come to think of it, you have 4 competing bugs here. That makes it a bit harder to sort out because you can fix any one of them and the program will probably still die. — user4581301, Jul 23 '17 at 20:45
Stop using pointers, it just adds orders of magnitude of complexity to your code for no reason other than " I dont know how to do stuff without pointers " — M.M, Jul 23 '17 at 21:35

user4581301 · Answer 1 · 2017-07-24T15:25:19.063

There are several bugs in the code intermingling and feeding off one another to make isolating any one of the bugs difficult. It's easy to fix one of the bugs and not notice it because another bug promptly takes its place.

Bugs 1 and 2 are likely immediately fatal and what OP is seeing as they have to do with accessing invalid memory. Column (bug 1) is not being allocated enough storage and Input (bug 2) is not being allocated any storage. Bug 3, PrevLayer passed by value, is a nuisance, but not immediately fatal due to bug 4 (inadequate destuctor results in memory leak) negating bug 5 (Rule of Three violation).

We'll start with bug 3 because it is represented in the code first even though it will be seen after. Plus it's a really quick fix.

Layer::Layer(int LayerSize, Layer PrevLayer){

Bug 3 is PrevLayer is passed by value. This makes a copy of the source Layer and any modifications to PrevLayer will not be seen in the source Layer. Solution: Pass by reference. Layer(int LayerSize, Layer & PrevLayer)

    Size=LayerSize;

Here is bug 1:

    Column=(Neuron*)malloc(Size);

malloc allocates bytes, not objects. This gets you two ways: 1. Neuron's constructor is not run, so you have uninitialized Neurons. 2. You have Size bytes, not Size Neurons. Neuron's size is greater than a byte, so you haven't allocated enough memory.

Solution: Use std::vector<Neuron> Column; This is C++ after all, so there's no need to allocate memory like you would in a C program.

Bug 2 needs to be addressed right around here as well. No storage has been allocated for Input. Solution: std::vector<double*>; But note: this doesn't do you many favours with respect to speed on a modern processor and allows the possibility that the pointers can be rendered invalid due to a change in the size of Column. You are much better off just getting the value from Column as required.

    for(int i=0;i<Size;i++){
        Column[i]=Neuron(LayerSize,LayerSize,LayerSize);

Bug 2 will be exposed here

        Input[i]=&Column[i].Input;                        //1
        PrevLayer.Output[i]=Input[i];                     //2
    }
}

Bugs 3, 4 and 5: Already solved by using std::vector to solve bugs 1 and 2.

Alright. So how do we put all these solutions together?

class Neuron{
public:
    Neuron(int PrevColumnSize, int ThisColumnSize, int NextColumnSize);
    double Input;           //input of each neuron
};


class Layer{
    // Size is gone. vector knows how big it its.
    std::vector <Neuron> Column; //Column of Neurons
    std::vector <double*> Output; // really bad idea. Strongly recommend a rethink
public:
    Layer(int LayerSize);   
    Layer(int LayerSize, Layer &PrevLayer); // now takes a reference
    double operator[](size_t index) // to safely access Input.
    {
        return Column[index].Input;
    }
};

Layer::Layer(int LayerSize, Layer &PrevLayer){
    for(int i=0;i<LayerSize;i++){
        Column.emplace_back(LayerSize,LayerSize,LayerSize);  
    }
    // these steps must be separated and Column must never change size or 
    // these pointers are toast
    for (Neuron & column: Column)
    {
        PrevLayer.Output[i]=&column.Input; // This has a lot of potential for failure
                                           // Strongly recommend a rethink
    }
}

The Rule of Three and all destruction logic is handled by std::vector

Documentation on std::vector

Now that I've bashed all this out, I question the need for a Neuron class. A vector of double and a function to calculate input is all that appears to be required.

Can't assign an member variable address to a pointer

1 Answers1