[C++] destruktor klasy i core dumped


(Wojcirej) #1

OS: lubuntu 12.10

Kompilator: GNU GCC Compiler (język C++)

IDE: Code Blocks 10.05

Mam sobie oto taki konstruktor klasy:

Macierz::Macierz(int w, int k, int z)

{

    wiersze=w;

    kolumny=k;

    zakres=z;

    macierz=new int *[wiersze];

    for(int i=0;i
    {

        macierz[i]=new int[kolumny];

    }

}

Teraz chcę napisać do niego poprawny destruktor, który będzie zwalniał alokowaną pamięć dla tejże macierzy. Początkowo korzystałem z "pustego":

Macierz::~Macierz(void)

{

}

i wszystko działało jak należy. Jednak w obawie, że nastąpi wyciek pamięci, główkuję nad własną definicją destruktora (jest to też projekcik na zaliczenie i stąd takie ambitne plany :wink: ). I wpadłem na taki oto pomysł:

Macierz::~Macierz(void)

{

    for(int i=0;i
    {

        delete []macierz[i];

    }

    delete[] macierz;

}

I podczas wykonywania funkcji zaprzyjaźnionej, np dodawania macierzy, zdefiniowanej następująco:

Macierz Dodaj(Macierz m1, Macierz m2)

{

    Macierz dodawanie(m1.wiersze,m1.kolumny,0);

    for(int i=0;i
    {

        for(int j=0;j
        {

            dodawanie.macierz[i][j]=m1.macierz[i][j]+m2.macierz[i][j];

        }

    }

    return dodawanie;

}

Wywołanie w mainie:

Dodaj(matrix1,matrix2).Wypisz();

Wynik otrzymuję poprawny, jednak tracę wartości macierzy matrix1 i matrix2 (pojawiają się jakieś dziwne, nie wiadomo skąd wartości). Natomiast apogeum jest przy wywołaniu funkcji zaprzyjaźnionej, która ma zmieniać wartości danych macierzy (np. ZmieńZnaki o takim kodzie:)

Macierz ZmianaZnaku(Macierz m1)

{

    for(int i=0;i
    {

        for(int j=0;j
        {

            m1.macierz[i][j]*=-1;

        }

    }

    return m1;

}

Wtedy wywala coś takiego:

***glibc detected***./Macierz: munmap_chunk(): invalid pointer: 0x08a7d040 ***

======= Backtrace: =========

/lib/i386-linux-gnu/libc.so.6(+0x75ee2)[0xb749bee2]

/lib/i386-linux-gnu/libc.so.6(+0x765c5)[0xb749c5c5]

/usr/lib/i386-linux-gnu/libstdc++.so.6(_ZdlPv+0x1f)[0xb7637adf]

/usr/lib/i386-linux-gnu/libstdc++.so.6(_ZdaPv+0x1b)[0xb7637b2b]

./Macierz[0x8049787]

./Macierz[0x8049e3d]

/lib/i386-linux-gnu/libc.so.6(__libc_start_main+0xf3)[0xb743f4d3]

./Macierz[0x8048ae1]

======= Memory map: ========

08048000-0804c000 r-xp 00000000 08:05 264831 /home/wojcirej/Dropbox/Macierz/bin/Debug/Macierz

0804c000-0804d000 r--p 00003000 08:05 264831 /home/wojcirej/Dropbox/Macierz/bin/Debug/Macierz

0804d000-0804e000 rw-p 00004000 08:05 264831 /home/wojcirej/Dropbox/Macierz/bin/Debug/Macierz

08a7d000-08a9e000 rw-p 00000000 00:00 0 [heap]

b73f8000-b73fa000 rw-p 00000000 00:00 0 

b73fa000-b7424000 r-xp 00000000 08:05 1573549 /lib/i386-linux-gnu/libm-2.15.so

b7424000-b7425000 r--p 00029000 08:05 1573549 /lib/i386-linux-gnu/libm-2.15.so

b7425000-b7426000 rw-p 0002a000 08:05 1573549 /lib/i386-linux-gnu/libm-2.15.so

b7426000-b75c9000 r-xp 00000000 08:05 1573508 /lib/i386-linux-gnu/libc-2.15.so

b75c9000-b75ca000 ---p 001a3000 08:05 1573508 /lib/i386-linux-gnu/libc-2.15.so

b75ca000-b75cc000 r--p 001a3000 08:05 1573508 /lib/i386-linux-gnu/libc-2.15.so

b75cc000-b75cd000 rw-p 001a5000 08:05 1573508 /lib/i386-linux-gnu/libc-2.15.so

b75cd000-b75d1000 rw-p 00000000 00:00 0 

b75d1000-b75ed000 r-xp 00000000 08:05 1573533 /lib/i386-linux-gnu/libgcc_s.so.1

b75ed000-b75ee000 r--p 0001b000 08:05 1573533 /lib/i386-linux-gnu/libgcc_s.so.1

b75ee000-b75ef000 rw-p 0001c000 08:05 1573533 /lib/i386-linux-gnu/libgcc_s.so.1

b75ef000-b76cb000 r-xp 00000000 08:05 1182796 /usr/lib/i386-linux-gnu/libstdc++.so.6.0.17

b76cb000-b76cc000 ---p 000dc000 08:05 1182796 /usr/lib/i386-linux-gnu/libstdc++.so.6.0.17

b76cc000-b76d0000 r--p 000dc000 08:05 1182796 /usr/lib/i386-linux-gnu/libstdc++.so.6.0.17

b76d0000-b76d1000 rw-p 000e0000 08:05 1182796 /usr/lib/i386-linux-gnu/libstdc++.so.6.0.17

b76d1000-b76d8000 rw-p 00000000 00:00 0 

b76ec000-b76f1000 rw-p 00000000 00:00 0 

b76f1000-b76f2000 r-xp 00000000 00:00 0 [vdso]

b76f2000-b7712000 r-xp 00000000 08:05 1573486 /lib/i386-linux-gnu/ld-2.15.so

b7712000-b7713000 r--p 0001f000 08:05 1573486 /lib/i386-linux-gnu/ld-2.15.so

b7713000-b7714000 rw-p 00020000 08:05 1573486 /lib/i386-linux-gnu/ld-2.15.so

bfc93000-bfcb4000 rw-p 00000000 00:00 0 [stack]

Przerwane (core dumped)

I teraz pytanie: Jak poprawnie zdefiniować destruktor, żeby wyeliminować oba te kłopoty?


Różnice między C,C# a C++
(Rolek0) #2

A masz konstruktor kopiujący, który zaalokuje nową pamięć?

Bo domyslny konstruktor kopiujący przepisuje dosłownie pola obiektu i masz 2 wskaźniki wskazujące na ten sam obszar, gdy niszczysz jeden obiekt, zwalniasz pamięć ale drugi wciąż ma wskaźnik na tą pamięć i przy próbie użycia masz błąd.


(Wojcirej) #3

Słuszna uwaga, poniekąd to rozwiązuje moje problemy. Oto jego definicja:

Macierz::Macierz(Macierz &m)

{

    wiersze=m.wiersze;

    kolumny=m.kolumny;

    zakres=m.zakres;

    macierz=new int *[wiersze];

    for(int i=0;i
    {

        macierz[i]=new int[kolumny];

    }

    for(int i=0;i
    {

        for(int j=0;j
        {

            *(*(macierz+i)+j)=*(*(m.macierz+i)+j);

        }

    }

}

I po wywołaniu funkcji zaprzyjaźnionej dodająca macierze wartości zostają. Więc dziękuję bardzo. :slight_smile: Jednak po wywołaniu funkcji zaprzyjaźnionej zmieniającej znaki nadal tracę wartości. Tak ją wywołuję:

matrix1=ZmianaZnaku(matrix1);

            matrix2=ZmianaZnaku(matrix2);

Czy muszę definiować kolejny konstruktor?


(Rolek0) #4

Przydałby się jeszcze operator= :wink:


(Wojcirej) #5

No i wszystko działa, jak należy. Widzę, że poległem na podstawach. :wink:

Wielkie dzięki za dotychczasową pomoc! :slight_smile:

Jedno pytanko jeszcze: czy ten destruktor, który zaproponowałem w pierwszym poście, jest poprawny od strony programistycznej?


(Rolek0) #6

Generalnie nie jest źle chociaż mógłbyś to lepiej zaprojektować, np. trzymać macierz w jednym kawałku pamięci a nie porozrzucaną w kawałkach (współczesne procesory są dużo szybsze od pamięci i częste cache-miss bywają bolesne dla wydajności).

Dobrze by też było, jeśli nie modyfikujesz argumentów, przekazywać je przez referencję do stałej aby uniknąć niepotrzebnego kopiowania (chyba, że zastosowałbyś optymalizację typu copy-on-write). Przy konstruktorze kopiującym referencja też powinna być do stałej, żebyś nie miał problemów ze zrobieniem kopii stałego obiektu.

Generalnie przy obiektach reprezentujących jakieś wartości jak np. twoje macierze należy mieć: konstruktor zwykły, konstruktor kopiujący, destruktor i operator przypisania. Od C++11 dobrze jest też mieć konstruktor przesuwający i przesuwający operator przypisania.

Oprócz tego mogą się przydać jeszcze inne operatory, np. zamiast twojej funkcji Dodaj mógłbyś mieć operator+ :smiley:


(Wojcirej) #7

Zaintrygowała mnie ta część o konstruktorze przesuwającym - jaka jest jego rola? Mógłbyś podrzucić jakieś źródło, gdzie można o tym poczytać?


(Rolek0) #8

Hmmm… źródło, dobre chyba będzie google: c++ move semantics :smiley:

Konstruktor przesuwający podobnie jak kopiujący ma utworzyć obiekt identyczny z oryginałem, natomiast oryginał po operacji przeniesienia ma być “pusty” (co to dokładnie znaczy zalezy od tego co ma reprezentować obiekt). Np. jak masz wskaźnik w obiekcie to konstruktor przesuwający zamiast kopiować pamięć kopiuje sam wskaźnik a w oryginale ustawia pusty wskaźnik. Podobną rolę ma przesuwający operator przypisania.

Jest to przydatne przy zwaracaniu obiektu z funkcji.

Foo bar()

{

	Foo x;

	return x;

}


Foo f = bar(); //może być użyty konstruktor przesuwający

f = bar(); //będzie użyty przesuwający operator=

Standardowo używane są konstruktor kopiujący i zwykły operator przypisania co może powodować niepotrzebne kopiowanie danych. Chociaż, przy tworzeniu obiektu na podstawie wartości zwracanej z funkcji standard pozwala kompilatorowi pominąć wywołanie konstruktora i destruktora i dosłownie przepisać zawartość obiektu a nawet od razu utworzyć dany obiekt w docelowym miejscu i wiele kompilatorów to stosuje. Można też jawnie używać semantyki przeniesienia przy użyciu std::move

#include 

#include 

#include 

#include 


int main()

{

	std::vector v;

	std::string str = "Hello!";

	std::cout << "Zawartosc stringa: \"" << str << '\"' << std::endl;

	v.push_back(str);

	std::cout << "Zawartosc stringa po kopiowaniu: \"" << str << '\"' << std::endl;

	v.push_back(std::move(str));

	std::cout << "Zawartosc stringa po przeniesieniu: \"" << str << '\"' << std::endl;

	std::cout << "Zawartosc vectora: ";

	for(auto i = v.begin(); i != v.end(); ++i)

		std::cout << '\"' << *i << "\" ";

	return 0;

}

Deklaracje

T(T&& o);


T& operator=(T&& r); //metoda

Poprawka: operator przypisania nie może być funkcją globalną, musi być metodą, sorry za błąd #-o


(Wojcirej) #9

Czyli że w praktyce dla moich macierzy miałoby to wyglądać tak?

Nagłówek:

Macierz(const Macierz &&m);

Definicja:

Macierz::Macierz(const Macierz &&m)

{

    wiersze=m.wiersze;

    kolumny=m.kolumny;

    zakres=m.zakres;

    macierz=m.macierz;

    m.macierz=NULL;

}

(Rolek0) #10

Przy przesuwaniu oryginał jest modyfikowany, więc nie może być const. Do pary jeszcze dopisz przesuwający operator przypisania i będzie git :smiley:

PS. W konstruktorach mógłbyś stosować listy inicjalizacyjne, co prawda gdy pola są typów prostych to nie ma różnicy ale przy typach bardziej złożonych możesz zaoszczędzić trochę cykli procesora :smiley: Po za tym pola stałe i będące referencjami można inicjalizować tylko w ten sposób.


(Wojcirej) #11

Dzisiaj dobrałem się do swojego kompilatora i po próbie skompilowania tego konstruktora pojawia mi się błąd:

error: expected ‘,’ or ‘...’ before ‘&&’ token|

error: invalid constructor; you probably meant ‘Macierz (const Macierz&)’|

Jeżeli to nie jest żaden błąd składniowy, to zapewne mój kompilator nie obsługuje jeszcze tego nowego standardu. :wink: Więc na razie odpuszczę, ale i tak wielkie dzięki, człowiek przynajmniej czegoś nowego się nauczył. :slight_smile: Tak w ciemno zapytam - przesuwający operator przypisania miałby wyglądać tak?

Macierz &operator=(Macierz &&m)

{

    wiersze=m.wiersze;

    kolumny=m.kolumny;

    zakres=m.zakres;

    macierz=m.macierz;

    m.macierz=NULL;

    return *this;

}

Bo mi to tak wychodzi, że robota ta sama. :wink: I jeszcze chciałbym prosić o poradę, bo mam problem z kolejnym operatorem - tym razem dodawania. Nagłówek:

Macierz &operator +(const Macierz &m);

Definicja:

Macierz &Macierz::operator+(const Macierz &m)

{

    Macierz suma(m);

    for(int i=0;i
    {

        for(int j=0;j
        {

            suma.macierz[i][j]=macierz[i][j]+m.macierz[i][j];

        }

    }

    return suma;

}

I przy następującej próbie wywołania:

wynikowa=matrix1+matrix2;

Otrzymuję błąd 139 - Segmentation fault (Core dumped). Co znowuż robię nie tak? Szczerze powiem, że operatory inne niż << oraz = sprawiały mi kłopot. :confused:


(Rolek0) #12

Z tego co wiem GCC obsługuje C++11, sprawdź czy masz najnowszą wersję. Musisz też podać odpowiedni argument aby używał nowego standardu, bodajże -std=cośtam (poczytaj w manualu).

Operator przypisania powinien sprawdzać czy nie próbujesz przypisać obiektu do niego samego.

if(this == &m)

	return *this;

Powinien też usuwać dotychczasową zawartość obiektu docelowego przed przepisaniem danych.

Funkcje tworzące nowy obiekt powinny ten obiekt zwracać. Ty zwracasz referencję do obiektu lokalnego, który jest niszczony przy wyjściu z funkcji.

Przy okazji, co się stanie jesli dodasz do siebie macierze o różnych wymiarach? :wink:


(Wojcirej) #13

Rzeczywiście, gcc obsługuje standard C++11, wystarczyło zaptaszyć odpowiednia opcję. Wszystko kompiluje się, jak należy. Z działaniem już gorzej - po implementacji tego konstruktora przesuwającego:

Macierz::Macierz(Macierz &&m):wiersze(m.wiersze),kolumny(m.kolumny),zakres(m.zakres)

{

    macierz=m.macierz;

    m.macierz=NULL;

}

oraz przesuwającego operatora przypisania:

Macierz &Macierz::operator=(Macierz &&m)

{

    if(this==&m) return *this;

    for(int i=0;i
    {

        delete []macierz[i];

    }

    delete[] macierz;

    wiersze=m.wiersze;

    kolumny=m.kolumny;

    zakres=m.zakres;

    macierz=m.macierz;

    m.macierz=NULL;

    return *this;

}

Dostaję Segmentation fault (Core dumped). Nie działają nawet stare metody, które działały przed implementacją powyższych dwóch. Odnośnie dodawania macierzy - sprawdzanie wymiarów odbywa się w mainie, dopiero po wyborze opcji wyboru dodawania macierzy, dlatego nie definiowałem tego w metodzie.

if(matrix1.ZwrocKolumny()!=matrix2.ZwrocKolumny())

            {

                cout<<"Tych macierzy nie mozna dodawac!"<
            }

            else if(matrix1.ZwrocWiersze()!=matrix2.ZwrocWiersze())

            {

                cout<<"Tych macierzy nie mozna dodawac!"<
            }

Więc pozostaje tylko kwestia poprawnej definicji operatora. Chciałem zwracać w nim this=suma, ale wówczas wartości pierwszej macierzy były zastępowane sumą, więc to nie był najlepszy pomysł. Dlatego pomyślałem, że jeśli zrobię to z wywołaniem, zadziała przeciążony operator przypisania.


(Rolek0) #14

Hmmm… A sprawdzasz w destruktorze czy wskaźnik nie jest null?

W operator= przed usunięciem dotychczasowej zawartości i w ogóle gdzie się odwołujesz do danych pod wskaźnikiem też wypadałoby to na wszelki wypadek sprawdzić.

Co do tego ifa to

if(matrix1.ZwrocKolumny()!=matrix2.ZwrocKolumny() || matrix1.ZwrocWiersze()!=matrix2.ZwrocWiersze())

{

	cout<<"Tych macierzy nie mozna dodawac!"<
}

:wink:


(Wojcirej) #15

Z ifem racja, zawsze to jedno zagnieżdżenie mniej. :wink:

Poprawiłem operatory = na następujący zapis:

Macierz &Macierz::operator=(Macierz &&m)

{

    if(this==&m) return *this;

    while(macierz!=NULL)

          {

              for(int i=0;i
                {

                delete []macierz[i];

                }

            delete[] macierz;

          }

    wiersze=m.wiersze;

    kolumny=m.kolumny;

    zakres=m.zakres;

    macierz=m.macierz;

    m.macierz=NULL;

    return *this;

}

Macierz &Macierz::operator=(const Macierz &m)

{

    while(macierz!=NULL)

    {

        for(int i=0;i
        {

            delete []macierz[i];

        }

        delete[] macierz;

    }

    wiersze=m.wiersze;

    kolumny=m.kolumny;

    zakres=m.zakres;

    macierz=new int *[wiersze];

    for(int i=0;i
    {

        macierz[i]=new int[kolumny];

    }

    for(int i=0;i
    {

        for(int j=0;j
        {

            *(*(macierz+i)+j)=*(*(m.macierz+i)+j);

        }

    }

    return *this;

}

Macierz::~Macierz(void)

{

        while(macierz!=NULL)

          {

              for(int i=0;i
                {

                delete []macierz[i];

                }

            delete[] macierz;

          }

}

I teraz dla odmiany dostaję 134. Chyba mam coś zdrowo pomieszane od tych nowych metod. :frowning:


(Rolek0) #16

Eeee… ale wiesz co robi while? :wink:


(Wojcirej) #17

O rany… ale wtopa! !!

Jeszcze tylko ten operator dodawania… Czy chodzi o to, żeby operator zwracał stworzony obiekt? Wtedy potrzebowałbym kolejnego konstruktora, który dodawałby.


(Rolek0) #18

Tak.

??? Przy zwracaniu obiektu z funkcji teoretycznie działają konstruktory kopiujące lub przesuwające, w praktyce standard pozwala to pominąć i pisać od razu tam gdzie trzeba. Żadne dodatkowe konstruktory nie są Ci potrzebne.

T foo();

Jeśli T jest klasą/strukturą to jest to równoznaczne mniej więcej

void foo(T* ret);

Przy czym na wskazywanej pamięci nie był wywoływany konstruktor T(). Funkcja powinna tam zapisać zwracany obiekt przy użyciu konstruktora kopiującego lub przesuwającego, w praktyce w prostych przypadkach kompilatory często umieszczają tam od razu zmienną lokalną, która ma być zwrócona.


(Wojcirej) #19

Zrobiłem to w ten sposób

Macierz &Macierz::operator+(const Macierz &m)

{

    Macierz *suma=new Macierz(this->wiersze,this->kolumny,0);

    for(int i=0;iwiersze;++i)

    {

        for(int j=0;jkolumny;++j)

        {

            suma->macierz[i][j]=this->macierz[i][j]+m.macierz[i][j];

        }

    }

    return *suma;

}

I pyknęło, wszystko śmiga. :smiley: O to dokładnie chodziło? Analogicznie można przeciążyć chyba - i *, prawda?


(Rolek0) #20

Jeśli będziesz używał tego operatora tak jak zwykle normalnie używa się + to będziesz miał wyciek pamięci :wink:

Zrób po prostu

Macierz Macierz::operator+(const Macierz &m)

{

	Macierz suma(this->wiersze,this->kolumny,0);

	//...

	return suma;

}

I nie przejmuj się tym co kompilator zrobi pod spodem :wink:

Tak, przy czym istnieją po dwie wersje tych operatorów.

Binarny - to odejmowanie.

Unarny - to element przeciwny, tak jak np. 1 i -1.

Binarny * to mnożenie.

Unarny * to dereferencja, dla macierzy taki operator raczej nie miałby sensu :biggrin:

Dla sysmetrii z unarnym - istnieje też unarny +, oznaczający ten sam element, jest to chyba najmniej użyteczny operator, (przynajmniej w oryginalnym zachowaniu) :stuck_out_tongue:

Przy czym jest technicznie możliwe by operatory wykonywały inne zadanie niż zostały pierwotnie przeznaczone, np. strumienie używają operatorów << i >>, które oryginalnie oznaczają przesunięcia bitowe.