Should member pointers always use new in their initialization?

105 views Asked by At

In this question, I get weird segmentation fault in a method the constructor invokes, but apparently nowhere else (not even in a CodeBreaker::testVectorOfElements called from main() whose definition looks like:

void CodeBreaker::testVectorOfElements(int size)
{
    // try to do what you did in makeNumericCodes with the vector
    while (this->numericCodes.size() < size)
    {
        this->numericCodes.push_back(this->Z[0]);
    }
}

CodeBreaker has a private std::vector<IntegerRing::Element> numericCodes.

The source of this problem seems to be with my declaration of IntegerRing, which is an IntegerGroup. The MCVE code to both classes can be found below:

IntegerGroup.h

#ifndef INTEGERGROUP_H
#define INTEGERGROUP_H

#include "Array.h"
#include <vector>   // for some reason, this didn't work.

#include <iostream>

// This group is the integers mod n
// multiplication in integer group is simply integer addition modulo n
class IntegerGroup
{
    public:
        IntegerGroup();
        IntegerGroup(int);
        class GroupElement
        {
            protected:
                int m;
                IntegerGroup* group;
            public: 
                GroupElement();
                GroupElement(int);
                GroupElement(int, IntegerGroup*);
                ~GroupElement();
        };
        int size() const;
        protected:
            int n;
            Array<GroupElement> elements;
            void createNewElement(int);
};

#endif

IntegerGroup.cpp

#include "IntegerGroup.h"

#include <new>
#include <iostream>

IntegerGroup::IntegerGroup()
{

}

IntegerGroup::IntegerGroup(int n)
 : n(n), elements(Array<IntegerGroup::GroupElement>(n))
   //elements(std::vector<IntegerGroup::GroupElement>(n))
{
    //this is to have integers in [0,n-1]
    for (int j = 0; j < n; j++)
    {
        this->createNewElement(j);
    }
}

void IntegerGroup::createNewElement(int m)
{
    // create new GroupElement
    GroupElement newElement(m, this);
    // store it at index m in elements
    this->elements[m] = newElement;
}

IntegerGroup::GroupElement::GroupElement() 
    : group(0)
{

}

IntegerGroup::GroupElement::GroupElement(int x)
    : m(x), group(0)
{

}

IntegerGroup::GroupElement::GroupElement(int m, IntegerGroup * g)
    : group(g)
{
    // this->m must be in [0, g->size() - 1], if g not null
    if (g)
    {
        this->m = m % g->size();
        if (this->m < 0) this->m = g->size() + this->m;
    }
    else
    {
        this->m = m;
    }
}

IntegerGroup::GroupElement::~GroupElement()
{
    if (this->group)
    {
        this->group = 0;
    }
}

int IntegerGroup::size() const { return this->n; }

IntegerRing.h

#ifndef INTEGERRING_H
#define INTEGERRING_H

#include "IntegerGroup.h"
class IntegerRing : public IntegerGroup
{
    public:
        IntegerRing();
        IntegerRing(int);
        IntegerRing(const IntegerGroup&);
        class Element : public IntegerGroup::GroupElement::GroupElement
        {
            public: 
                Element();
                Element(int, IntegerGroup*);
                //~Element();
                operator IntegerGroup::GroupElement() { return IntegerGroup::GroupElement(); }
                Element(const IntegerGroup::GroupElement&);
        };
};

#endif

IntegerRing.cpp

#include "IntegerRing.h"
#include "IntegerGroup.h"


#include <iostream>

IntegerRing::Element::Element()
    : IntegerGroup::GroupElement()
{

}

IntegerRing::IntegerRing(int n)
    : IntegerGroup::IntegerGroup(n)
{

}

IntegerRing::Element::Element(int m, IntegerGroup * g)
    : IntegerGroup::GroupElement(m, g)
{

}

IntegerRing::Element::Element(const IntegerGroup::GroupElement& el)
    : IntegerGroup::GroupElement(el)
{

}

CodeBreaker.h

#ifndef CODEBREAKER_H
#define CODEBREAKER_H

#include <string>
#include <map>
#include <utility>
#include <vector>

#include "IntegerRing.h"
#include "PunctuationOccurrenceList.h"

class CodeBreaker
{
    public:
        CodeBreaker();
        CodeBreaker(std::string&, bool);
        void testVectorOfElements(int);
    private:
        PunctuationOccurrenceList punctuation;
        IntegerRing Z;
        void initCharMap();
        void makeNumericCodes();
        int getNumericCodeFor(char) const;
        std::map<char, int> charFrequencyMap;
        std::vector<IntegerRing::Element> numericCodes;
        std::string ciphertext, plaintext;
        std::string cipherFilename;
};

#endif

CodeBreaker.cpp

#include "CodeBreaker.h"

#include <iostream>
#include <fstream>
#include <algorithm>

CodeBreaker::CodeBreaker()
    : Z(IntegerRing::IntegerRing(26)),
        punctuation(PunctuationOccurrenceList::PunctuationOccurrenceList())
{
    // initialize charFrequencyMap
    this->initCharMap();
}

CodeBreaker::CodeBreaker(std::string& str, bool readFromFile)
    : Z(IntegerRing::IntegerRing(26))
{
    // if reading from file
    if (readFromFile)
    {
        // str is filename to read from 
        // open file specified by str
        std::ifstream input(str.c_str());
        std::string line;
        // read contents to ciphertext
        while (std::getline(input, line))
        {
            this->ciphertext += line + "\n";
        }
        // close file
        input.close();
    }
    // otherwise
    else
    {
        // str is ciphertext itself
        // write it to this->ciphertext
        this->ciphertext = str;
    }
    // strip punctuation
    this->punctuation = PunctuationOccurrenceList::PunctuationOccurrenceList(this->ciphertext);
    // log numeric codes ('A' => 0, 'B' => 1, ... , 'Z' => 25)
    this->makeNumericCodes();
}

void CodeBreaker::makeNumericCodes()
{
    std::cout << "this->ciphertext.size() == " << this->ciphertext.size() << std::endl;
    std::cout << "this->numericCodes.capacity() == " << this->numericCodes.capacity() << std::endl;
    std::cout << "this->numericCodes.max_size() == " << this->numericCodes.max_size() << std::endl;
    // if there isn't as many numeric codes as there are characters in ciphertext
    while (this->numericCodes.size() < this->ciphertext.size())
    {
        std::cout << "Pushing dummy Element to numericCodes...\n";
        this->numericCodes.push_back(this->Z[0]); 
    }
    // for every character in ciphertext
    for (int j = 0; j < this->ciphertext.size(); j++)
    {
        // if character's numerical code is not logged in numericCodes
        int numericCode = this->getNumericCodeFor(ciphertext[j]);
        if (numericCode != this->numericCodes[j].val())
            // log it
            this->numericCodes[j] = (IntegerRing::Element)this->Z[numericCode];

    }
    // while there are more numericCodes than characters in ciphertexts
    while (this->numericCodes.size() > this->ciphertext.size())
        // remove last character
        this->numericCodes.pop_back();
}

void CodeBreaker::testVectorOfElements(int size)
{
    // try to do what you did in makeNumericCodes with the vector
    while (this->numericCodes.size() < size)
    {
        this->numericCodes.push_back(this->Z[0]);
    }
}

int CodeBreaker::getNumericCodeFor(char c) const
{
    // if character is in ['A', 'Z']
    if ((c >= 'A') && (c <= 'Z'))
        // return its int value minus 'A'
        return (int)(c - 'A');
    // otherwise, if it is in ['a', 'z']
    if ((c >= 'a') && (c <= 'z'))
        // return its int value minus 'a'
        return (int)(c - 'a');
    std::cout << "invalid character: " << c << std::endl;
    return -1;
}

void CodeBreaker::initCharMap()
{
    for (char c = 'A'; c <= 'Z'; c++)
    {
        this->charFrequencyMap[c] = 0;
    }
}

main.cpp

#include <iostream>
#include <string>

#include "CodeBreaker.h"
//#include "IntegerRing.h"


using namespace std;

int main()
{
    bool testInClass = true;
    if (testInClass)
    {
        string str = "I'm plaintext, but deal with me anyways, because this is mock code.";
        CodeBreaker c(str, false);      
        /*CodeBreaker d;
        d.testVectorOfElements(64);*/
    }
    else
    {
        vector<IntegerRing::Element> a;
        IntegerRing Z(26);
        cout << "a.capacity == " << a.capacity() << endl;
        while (a.size() < 2400)
        {
            a.push_back(Z[0]);
        }
        cout << "a.size() == " << a.size() << endl;
        for (int j = 0; j < 26; j++)
        {
            cout << a[j] << " ";
        }
        cout << endl;
    }
    return 0;
}

Array is simply a DIY-vector that I had to write years ago for a data-structures class. I used it instead of vector for precisely the problem I am having here. (Problem-ception?)

All the code is available here, and the question is if I should have used new for when I assign those pointers? When IntegerRing is created, it tries creating IntegerRing::Elements, to which it passes a pointer to itself (out of necessity: the mathematical definition of ring requires it be equipped with addition and multiplication, and elements of that mathematical structure use that operator).

0

There are 0 answers