0

I get sigtrap (trace/brakepoint trap) error in my code c++. I have a generic list that I use for storing teams. I have one place where I use dinamic memory allocation. My list:

#ifndef LISTOFITEMS_HPP
#define LISTOFITEMS_HPP

#include <iostream>
template <class T>
class Csapatok
{
    struct ListaElem
    {
        T adat;
        ListaElem *kov;
    };

    ListaElem *elso;
public:
    Csapatok() : elso(nullptr) {}

    void beszur(const T& dat)
    {
        ListaElem *p = new ListaElem;
        p->adat = dat;
        p->kov = nullptr;

        if(elso == nullptr)
        {
            elso = p;
        }
        else
        {
            ListaElem* akt = elso;
            while (akt->kov != nullptr)
            {
                akt = akt->kov;
            }
            akt->kov = p;
        }
    }

    void torol(const T dat)
    {
        ListaElem* aktualis = elso;
        ListaElem* elozo = nullptr;
        if (aktualis != nullptr && aktualis->adat == dat)
        {
            elso = aktualis->kov;
            delete aktualis;
            return;
        }
        while (aktualis != nullptr && aktualis->adat != dat)
        {
            elozo = aktualis;
            aktualis = aktualis->kov;
        }
        if(aktualis == nullptr)
        {
            return;
        }
        if (elozo == nullptr)
        {
            elso = aktualis->kov;
        }
        else
        {
         elozo->kov = aktualis->kov;
        }
        delete aktualis;
    }
    bool keres(T dat) const
    {
        if(elso->adat == dat)
            return true;
        ListaElem *p = elso;
        while (p->kov != nullptr)
        {
            if(p->adat == dat)
                return true;
            p = p->kov;
        }
        return false;
    }
};

#endif //LISTOFITEMS_HPP


And this is my code for the Teams class:

#ifndef EGYESULET_H
#define EGYESULET_H

#include <iostream>
#include <cstring>
class Csapat{
protected:
    size_t alapletszam = 10;
    size_t letszam =0;
    char *csapetnev;
public:
    Csapat() :csapetnev(nullptr) {}
    Csapat(const char* csnev)  {
        if (csnev != nullptr) {
            csapetnev = new char[strlen(csnev) + 1];
            strcpy(csapetnev, csnev);
        } else {
            csapetnev = nullptr;
        }
    }
    const char* getcsnev() { return csapetnev;}
    ~Csapat()
    {
        delete[] csapetnev;
        std::cout << "csapat dtor"<< std::endl;
    }
};
#endif //EGYESULET_H

I have 3 derived classes from Teams (like basketball team, football team etc...) but they are so simple I dont put it here.

I have a testing file where I simply create an football team object f1. Then I put this team in the list using "beszur" function, and then checking it with "keres" if it is in or not. Somewhy it gives false when it should be true because I can see it in the debugger that it is in the list in its elso node. Then I try to delete it with "torol". Thats when I get a SIGTRAP error in the debugger at the line "delete[] csapatok".

I assume theres something with the destructors but I just cant handle it.

Máté Hermann
  • 1
  • Is there a reason you don't use [std::list](https://en.cppreference.com/w/cpp/container/list). So you can focus on modeling your other classes? I also see you use manually allocated char* for your texts, change your code to use std::string to hold strings. All the manual memory managment will increase the chance for bugs/crashes. The other piece of advise : use a debugger – Pepijn Kramer May 28 '23 at 07:03
  • `Csapat` does not obey [the rule of three](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three). – molbdnilo May 28 '23 at 07:05
  • Add `Csapat(Csapat&&) = delete; Csapat& operator=(Csapat&&) = delete;` to `Csapat` and you'll get compiler errors in the places that'll result in a double free in the end. (Any copy of an `Csapat` object results in shared ownership of the array resulting in a double free... Do yourself a favor and use `std::string` instead of the array and you'll get copy&move semantics for your class for free... (If you do that you'll need to remove the deleted special members of course for copy&move to be autogenerated.) – fabian May 28 '23 at 07:09
  • This is a school project and thats why I cant use STL containers. Edit: Nor I can use std:string. – Máté Hermann May 28 '23 at 07:17
  • @MátéHermann And that is where you teacher is wrong. It will mean you will learn something that is frowned upon in industry because it leads to bugs (as you are finding out). Datastructures can be taught fine even without manual memory managment. Tell him to adapt his course by using the [C++ core guidelines](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines). Specialy rule R.11 – Pepijn Kramer May 28 '23 at 07:21
  • 1
    @MátéHermann OK, but you still need to follow the [rule of three](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) to fix your bugs. I completely agree with the previous comment. What exactly does your teacher think he is teaching you by forbidding you from writing code in the way that any competent C++ programmer would do, Even if the point of the exercise is to learn how to program a linked list that is no reason to say you cannot use `std::string`. – john May 28 '23 at 07:22
  • So for the rule of three as i understand I should forbid my Csapat class from copying by placing the copy constructor in private? – Máté Hermann May 28 '23 at 07:26
  • @MátéHermann Either you should implement a correct copy constructor **and** assignment operator, or you should make them both private (or preferably deleted). However a class that cannot be copied is crippled, so normally you should take the first option – john May 28 '23 at 07:28
  • @MátéHermann And looking at your linked list class you do need to copy `Csapat` so you really do need to implement those copy constructors and assignment operators. – john May 28 '23 at 07:31
  • I made the contstructor in this way: Csapat(const Csapat& cs) { if(cs.csapatnev != nullptr) { csapatnev = new char[strlen((cs.csapatnev)) + 1]; strcpy(csapatnev, cs.csapatnev); } alapletszam = cs.alapletszam; letszam = cs.letszam; } – Máté Hermann May 28 '23 at 07:39
  • And basically the = operator is the same, it just returns with a pointer: Csapat& operator=(const Csapat& cs){ if(cs.csapatnev != nullptr) { csapatnev = new char[strlen((cs.csapatnev)) + 1]; strcpy(csapatnev, cs.csapatnev); } alapletszam = cs.alapletszam; letszam = cs.letszam; return *this; } – Máté Hermann May 28 '23 at 07:40
  • After writing the copy contsructor and assignment operator (and changing the while loop in 'keres' to p != nullptr) It seems that now it is alright and it throws no error, not even in debugger. Thanks a lot. – Máté Hermann May 28 '23 at 07:57
  • Your 1st class seems to be a duplicate for `std::forward_list`; just use the standard class. Your second class is just a buggy duplicate for `std::string`. In short words you don't need pointers at all. – Red.Wave May 28 '23 at 08:35
  • Rule of 0/3/5 applies to every C++ class. In your case, both classes shoul follow rule of 3 for runtime correctness. Otherwise resource (in your case memory) leaks occur. A good C++ instructor would teach RAII prior to data structures. I guess he knows nothing. – Red.Wave May 28 '23 at 08:40
  • @MátéHermann *This is a school project and thats why I cant use STL containers. Edit: Nor I can use std:string.* -- And both `std::list` and `std::string` have been officially part of C++ for 25 years now. That is probably older than most people in your class, and maybe your teacher was a pre-teenager when these items were officially part of C++. So it now sounds silly that these items cannot be used, unless the teacher is going over how to *properly* put together a class that requires correct copy semantics (and not just parotting the line "you can't use STL") – PaulMcKenzie May 28 '23 at 09:12
  • @MátéHermann -- After reading what you wrote for the assignment operator, it is broken as it leaks memory. The assignment operator should be `Csapat& operator=(Csapat cs) {std::swap(cs.alapletszam, alapletszam); std::swap(cs.letszam, letszam); std::swap(cs.csapetnev, csapetnev); return *this; }`. This works only if the copy constructor and destructor for `Csapat` are working correctly. – PaulMcKenzie May 28 '23 at 09:20
  • Thanks for the note @PaulMcKenzie, I corrected my assignment operator. – Máté Hermann May 28 '23 at 15:28

0 Answers0