[C++] destruktor klasy i core dumped

Wojcirej · 27 Styczeń 2013 12:42

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 ). 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?

Rolek0 · 27 Styczeń 2013 12:54

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 · 27 Styczeń 2013 13:16

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. 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 · 27 Styczeń 2013 13:29

Przydałby się jeszcze operator=

Wojcirej · 27 Styczeń 2013 14:08

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

Wielkie dzięki za dotychczasową pomoc!

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

Rolek0 · 27 Styczeń 2013 14:49

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+

Wojcirej · 29 Styczeń 2013 15:15

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 · 29 Styczeń 2013 16:34

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

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 · 29 Styczeń 2013 18:56

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 · 29 Styczeń 2013 21:04

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

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 Po za tym pola stałe i będące referencjami można inicjalizować tylko w ten sposób.

Wojcirej · 30 Styczeń 2013 14:42

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. Więc na razie odpuszczę, ale i tak wielkie dzięki, człowiek przynajmniej czegoś nowego się nauczył. 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. 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.

Rolek0 · 30 Styczeń 2013 19:28

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?

Wojcirej · 30 Styczeń 2013 20:10

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 · 30 Styczeń 2013 20:23

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!"<
}

Wojcirej · 30 Styczeń 2013 20:36

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

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.

Rolek0 · 30 Styczeń 2013 20:40

Eeee… ale wiesz co robi while?

Wojcirej · 30 Styczeń 2013 20:48

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 · 30 Styczeń 2013 21:11

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 · 30 Styczeń 2013 22:05

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. O to dokładnie chodziło? Analogicznie można przeciążyć chyba - i *, prawda?

Rolek0 · 31 Styczeń 2013 13:58

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

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

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)

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.