Was haltet ihr von meiner eigenen CONTAINER Klasse?

Um was für einen Container handelt es sich denn? Wäre nett wenn du das dazugeschrieben hättest und uns nicht raten liesest.

P.S. natürlich hab ich sofort gesehen, dass es eine schlechte Implementation von einem dynamischen Array sein soll.

LukasBanana

*hust hust* XD

okay, also eigentlich habe ich nur mal eine eigene Container Klasse geschrieben um mich mehr mit pointern usw. auseinander zu setzten. Ich hatte natürlich nie vor die STL zu verbessern.

Ich hab den Thread hier halt aufgemacht weil ich das nicht extra zu Projekten stellen wollte, aber ich wollte lediglich wissen ob das soweit ganz gut, oder eben wie sich herausgestellt hat schlecht aus sieht.

Trotzdem danke für eure Kritik

Don06

Du solltest in Erwägung ziehen, dass Ganze in etwa so zu implementieren:

// 
// ============ math -> Vector class ============ 
// 
// Copyright (c) 2008 Lukas Hermanns 
// 

#ifndef MATH_VECTOR
#define MATH_VECTOR 

#include <vector>

namespace math 
{
    using std::vector;
}
#endif // MATH_VECTOR

Da kannst du weniger Fehler machen

Gruß
Don06

#ifndef MATH_VECTOR
#define MATH_VECTOR

#include <vector>

template class<T> 
class math : public std::vector<T>{};

#endif // MATH_VECTOR

Nexus

MontagMorgen schrieb:

class math : public std::vector<T>{};

Es gab hier schon mehrere Diskussionen darüber, dass man nicht von STL-Containern erben sollte (u.a. wegen nicht vorhandenem virtuellen Destruktor).

@ LukasBanana
Ich würde mich noch einmal genau mit den Grundlagen auseinander setzen. Wenn du einen sauberen C++-Container erstellen willst, kannst du z.B. auch auf memcpy verzichten. Ausserdem ist ein Iterator nicht das Gleiche wie ein Zeiger; wenn du schon Typedefs machen willst, nenn sie wenigstens pointer und const_pointer . Allerdings finde ich das sowieso nicht sehr angebracht, zumal man einfach mit Hilfe des Adressoperators und der Rückgabefunktionen der jeweiligen Elemente auch einen Zeiger auf ein Element bekommt...

Und bevor du den Container mit irgendwelchen Funktionen ausbaust, solltest du zuerst einige wichtige Methoden richtig implementieren ( push_back() , pop_back() , erase() , insert() , size() , clear() ) plus Konstruktoren, op= und Destruktor. Teste den Container auch ausreichend.

Hey ihr kaggn00bs für mathematische Vektoren gibts std::valarray, oh Mann was für n00bs ihr seid, ey!

Shade Of Mine

Nexus schrieb:

Ich würde mich noch einmal genau mit den Grundlagen auseinander setzen. Wenn du einen sauberen C++-Container erstellen willst, kannst du z.B. auch auf memcpy verzichten. Ausserdem ist ein Iterator nicht das Gleiche wie ein Zeiger; wenn du schon Typedefs machen willst, nenn sie wenigstens pointer und const_pointer . Allerdings finde ich das sowieso nicht sehr angebracht, zumal man einfach mit Hilfe des Adressoperators und der Rückgabefunktionen der jeweiligen Elemente auch einen Zeiger auf ein Element bekommt...

dir ist schon klar dass es gängig ist auf der höchsten optimierungsstufe iteratoren in vectoren als reine zeiger zu implementieren, oder?

LordJaxom

Oder anders ausgedrückt: Der Iterator ist ein Konzept, der das Konzept des Zeigers (auf Arrayelemente) auf beliebige Container abstrahiert. Bei allen Anforderungen, die an Iteratoren gestellt werden können, ist der nackte Zeiger sogar der einfachste mögliche Iterator, der jede einzelne davon erfüllt.

Nexus

Shade Of Mine schrieb:

dir ist schon klar dass es gängig ist auf der höchsten optimierungsstufe iteratoren in vectoren als reine zeiger zu implementieren, oder?

Ich wollte nur das potenzielle Missverständnis vermeiden, dass Iteratoren generell immer nur Zeiger (mit deren Arithmetik) wären - auch wenn es bei std::vector so sein mag, ist es nicht bei allen Containern der Fall. Ich bin mir auch bewusst, dass es hier um ein vector-ähnliches Gebilde geht. Aber demnach ist der Bezeichner iterator hier durchaus gerechtfertigt...

LukasBanana

Also ich hab das ganze noch mal ein bischen überarbeite

Ich habe an einigen Stellen auf eure Tipps bzw. Kritik gehört und hoffe dass meien Klasse jetzt schon eher ansprechend aussieht

Erneuerungen:

(* ein paar Namensänderungen)
* delete [] data_
* "std::copy" & "std::fill" statt "memcpy" & "memset"
* zusätzlich: const T& operator [] (u32 index) const { /* ... */ }
* kein NULL mehr in Benutzung
* "push_back" & "push_front" statt "add"
* eigene 'typedefs' wie "u32" statt "unsigned int"

Hier ist die neue Klasse:

//
// ============ math -> Container class ============
//
// Copyright (c) 2008 Lukas Hermanns
//

#ifndef __MATH_CONTAINER__
#define __MATH_CONTAINER__

#include <algorithm>
#include <cstdlib>
#include <math.h>

typedef char                c8;
typedef wchar_t             c16;

typedef signed char         s8;
typedef signed short        s16;
typedef signed int          s32;
typedef signed long int     s64;

typedef unsigned char       u8;
typedef unsigned short      u16;
typedef unsigned int        u32;
typedef unsigned long int   u64;

typedef float               f32;
typedef double              f64;

namespace dim
{

template <typename T> class array
{

    public:

        /* Type definitions */

        typedef T*          iterator;
        typedef T* const    const_iterator;

        /* Constructors */

        array() : data_(0), len_(0)
        {
        }

        array(u32 size) : data_(0), len_(0)
        {
            create(size);
        }

        /* Destructor */

        ~array()
        {
            clear();
        }

        /* Operators - comparision */

        bool operator == (array<T> other)
        {
            /* Check if the arrays are not emtpy */
            if (!data_ || !other.data_)
                return false;

            /* Loop the elements */
            for (u32 i = 0; i < len_; ++i)
            {
                if (data_[i] != other.data_[i])
                    return false;
            }

            /* Exit the function */
            return true;
        }

        bool operator != (array<T> other)
        {
            /* Check if the arrays are not emtpy */
            if (!data_ || !other.data_)
                return false;

            /* Loop the elements */
            for (u32 i = 0; i < len_; ++i)
            {
                if (data_[i] != other.data_[i])
                    return true;
            }

            /* Exit the function */
            return false;
        }

        /* Operators - settings / gettings */

        T& operator [] (u32 index)
        {
            return data_[index];
        }

        const T& operator [] (u32 index) const
        {
            return data_[index];
        }

        array<T>& operator = (const array<T> &other)
        {
            /* Delete the old memory */
            if (data_)
                delete [] data_;

            /* Allcoate new memory */
            data_ = new T[other.len_];
            len_ = other.len_;

            /* Copy the memory */
            std::copy(&other.data_[0], &other.data_[other.len_], &data_[0]);

            /* Return the reference of this object */
            return *this;
        }

        /* Functions */

        bool empty()
        {
            return !data_;
        }

        u32 size()
        {
            return len_;
        }

        const_iterator begin()
        {
            return data_ ? &data_[0] : 0;
        }

        const_iterator end()
        {
            return data_ ? &data_[len_] : 0;
        }

        void create(u32 size)
        {
            /* Delete the old memory */
            if (data_)
                delete [] data_;

            /* Allocate new memory */
            data_ = new T[size];

            /* Set the length of the memory */
            len_ = size;

            /* Initialize the new memory */
            std::fill(begin(), end(), (T)0);
        }

        void clear()
        {
            if (data_)
            {
                delete [] data_;
                data_ = 0;
            }
            len_ = 0;
        }

        void push_back(const T &obj)
        {
            /* Allocate a temporary memory */
            T* tmp = new T[len_ + 1];

            /* Copy the memory */
            std::copy(begin(), end(), &tmp[0]);

            /* Add the new object */
            tmp[len_] = obj;

            /* Delete the old memory */
            if (data_)
                delete [] data_;

            /* Use the new memory */
            data_ = tmp;
            ++len_;
        }

        void push_front(const T &obj)
        {
            /* Allocate a temporary memory */
            T* tmp = new T[len_ + 1];

            /* Copy the memory */
            ++tmp;
            std::copy(begin(), end(), &tmp[0]);
            --tmp;

            /* Add the new object */
            tmp[0] = obj;

            /* Delete the old memory */
            if (data_)
                delete [] data_;

            /* Use the new memory */
            data_ = tmp;
            ++len_;
        }

        void push_back(const T array[], u32 size)
        {
            for (register u32 i = 0; i < size; ++i)
                push_back(array[i]);
        }

        void push_front(const T array[], u32 size)
        {
            for (register u32 i = 0; i < size; ++i)
                push_front(array[i]);
        }

        void insert(const_iterator it, const T &obj)
        {
            /* Allocate a temporary memory */
            T* tmp = new T[len_ + 1];

            /* Copy the memory */
            std::copy(begin(), end(), &tmp[0]);

            /* Loop the old memory */
            for (register u32 i = len_; i > it - &data_[0]; --i)
                tmp[i] = data_[i-1];

            /* Add the new object */
            tmp[it - &data_[0]] = obj;

            /* Delete the old memory */
            if (data_)
                delete [] data_;

            /* Use the new memory */
            data_ = tmp;
            ++len_;
        }

        void erase(const_iterator it)
        {
            /* Allocate a temporary memory */
            T* tmp = new T[len_ - 1];

            /* Copy the memory */
            std::copy(begin(), end() - 1, &tmp[0]);

            /* Loop the old memory */
            for (register u32 i = it - &data_[0]; i < len_ - 1; ++i)
                tmp[i] = data_[i+1];

            /* Delete the old memory */
            if (data_)
                delete [] data_;

            /* Use the new memory */
            data_ = tmp;
            --len_;
        }

        void erase(const_iterator begin, const_iterator end)
        {
            /* Check if the data is not empty */
            if (!data_)
                return;

            /* Allocate a temporary memory */
            const u32 less = end - begin + 1;
            T* tmp = new T[len_ - less];

            /* Copy the memory */
            std::copy(begin(), end() - less, &tmp[0]);

            /* Loop the old memory */
            for (register u32 i = begin - &data_[0]; i < len_ - less; ++i)
                tmp[i] = data_[i+less];

            /* Delete the old memory */
            delete [] data_;

            /* Use the new memory */
            data_ = tmp;
            len_ -= less;
        }

        void erase_front()
        {
            /* Check if the data is not empty */
            if (!data_)
                return;

            /* Allocate a temporary memory */
            T* tmp = new T[len_ - 1];

            /* Save the old mamory */
            T* oldarray = data_;

            /* Move the old memory */
            ++data_;

            /* Copy the memory */
            std::copy(begin(), end() - 1, &tmp[0]);

            /* Delete the old memory */
            delete [] oldarray;

            /* Use the new memory */
            data_ = tmp;
            --len_;
        }

        void erase_back()
        {
            /* Check if the data is not empty */
            if (!data_)
                return;

            /* Allocate a temporary memory */
            T* tmp = new T[len_ - 1];

            /* Copy the memory */
            std::copy(begin(), end() - 1, &tmp[0]);

            /* Delete the old memory */
            delete [] data_;

            /* Use the new memory */
            data_ = tmp;
            --len_;
        }

        void move(const_iterator itbegin, const_iterator itend, const_iterator it)
        {
            /* Check if the data is not empty */
            if (!data_)
                return;

            /* Allocate a temporary memory */
            T* tmp = new T[len_];

            /* Copy the memory */
            std::copy(begin(), end(), &tmp[0]);

            /* Move the memory */
            register u32 i;
            for (i = 0; i <= itend - itbegin; ++i)
                data_[i + (it - &data_[0])] = tmp[i + (itbegin - &data_[0])];

            /* Move the rest of the memory */
            if (it <= itbegin)
            {
                for (i = 0; i < itbegin - it; ++i)
                    data_[i + (it - &data_[0]) + itend - itbegin + 1] = tmp[i + (it - &data_[0])];
            }
            else
            {
                for (i = 0; i < it - itbegin; ++i)
                    data_[i + (itbegin - &data_[0])] = tmp[i + (itend - &data_[0] + 1)];
            }

            /* Delete temporary meory */
            delete [] tmp;
        }

        void resize(u32 newsize)
        {
            /* Check if the data is not empty */
            if (!data_)
                return;

            /* Allocate a temporary memory */
            T* tmp = new T[newsize];

            /* Initialize the new memory */
            std::fill(&tmp[0], &tmp[newsize], (T)0);

            /* Copy the memory */
            const_iterator itend = (newsize > len_ ? end() : begin() + newsize);
            std::copy(begin(), itend, &tmp[0]);

            /* Delete the old memory */
            delete [] data_;

            /* Use the new memory */
            data_ = tmp;
            len_ = newsize;
        }

        void swap(array<T> &other)
        {
            /* Check if the data are not emtpy */
            if (!data_ && !other.data_)
                return;

            /* Allocate temporary memory */
            T* tmp = data_;
            u32 len = len_;

            /* Swap the memories */
            data_ = other.data_;
            other.data_ = tmp;

            /* Swap the lengths */
            len_ = other.len_;
            other.len_ = len;
        }

        void flip()
        {
            /* Check if the data is not empty */
            if (!data_)
                return;

            /* Allocate temporary memory */
            T* tmp = new T[len_];

            /* Fill the temporary memory */
            std::copy(begin(), end(), &tmp[0]);

            /* Loop and flip the array */
            for (register u32 i = 0; i < len_; ++i)
                data_[i] = tmp[len_ - i - 1];

            /* Delete the temorary memory */
            delete [] tmp;
        }

        void unique()
        {
            /* Loop the data - main */
            for (register u32 i = 0, j; i < len_; ++i)
            {
                /* Loop the data - sub */
                for (j = 0; j < len_;)
                {
                    if (i != j && data_[i] == data_[j])
                        erase(begin() + j);
                    else
                        ++j;
                }
            }
        }

        void sort()
        {
            /* Check if the data is not empty */
            if (!data_)
                return;

            /* Loop the data - main */
            for (register u32 i = 0, j; i < len_; ++i)
            {
                /* Loop the data - sub */
                for (j = 0; j < len_; ++j)
                {
                    /* Check which value is smaller */
                    if (data_[i] < data_[j] && i != j)
                    {
                        /* Then move it before the element with the bigger value */
                        move(begin()+i, begin()+i, begin()+j);
                    }
                }
            }
        }

    private:

        /* Members */

        T* data_;

        u32 len_;

};

} // /namespace math

#endif

Don06

typedef T* const    const_iterator;

Argh! Ich weiß nicht, ob du das Konzept von Iteratoren schon verstanden hast. Lies mal das.

EDIT:
Ich mach dir mal ne Liste von WTFs in deinem Code. Es besteht allerdings keine Grantie auf Vollständigkeit.

1.) #ifndef __MATH_CONTAINER__
2.) #include <math.h>
3.) Wozu die typedefs? Wenn dann in der Klasse selbst.
4.) typedef T* const    const_iterator;
5.) bool operator == (array<T> other) // Deklaration + Definition
6.) bool operator != (array<T> other) // Deklaration + Definition
7.) array<T>& operator = (const array<T> &other) // Selbstzuweisung?
8.) bool empty() // Deklaration
9.) u32 size() // Deklaration
10.) const_iterator begin() // Deklaration, einzige Version?
11.) const_iterator end() // Deklaration, einzige Version?
12.) std::fill(begin(), end(), (T)0); // Beachte, dass begin() + end() bei dir eig. const_iteratoren sind, (T)0 ? User defined types?
13.) if (data_) delete [] data_; // doppelt-gemoppelt hält besser, was?
14.) void erase(const_iterator begin, const_iterator end) // Folge-Fehler
15.) void swap(array<T> &other) // Viel zu umständlich, könnte ein Zwei-Zeiler sein.

.filmor

Nexus schrieb:

MontagMorgen schrieb:
class math : public std::vector<T>{};
Es gab hier schon mehrere Diskussionen darüber, dass man nicht von STL-Containern erben sollte (u.a. wegen nicht vorhandenem virtuellen Destruktor).

Das sollte meines Erachtens gerade ein Beispiel sein, in dem man es darf (weil die abgeleitete Klasse keine Membervariablen hat und ihr Destruktor damit trivial ist). Damit sie benutzbar ist, sollten aber noch die Konstruktoren implementiert sein.

It0101

Nimm mal deinen RL-Namen aus dem Sourcecode hier raus Reale Existenzen haben im Internet nix zu suchen

camper

.filmor schrieb:

Das sollte meines Erachtens gerade ein Beispiel sein, in dem man es darf (weil die abgeleitete Klasse keine Membervariablen hat und ihr Destruktor damit trivial ist).

Das konstituiert keinen trivialen Destruktor. Ein trivialer Destruktor ist an dieser Stelle ohnehin kein Kriterium.

.filmor

Sondern?

David_pb

[unsinn]

zeusosc

@Don06

array<T>& operator = (const array<T> &other) // Selbstzuweisung?

alternative??

Grüüüße

David_pb

zeusosc schrieb:

@Don06
array<T>& operator = (const array<T> &other) // Selbstzuweisung?
alternative??

Grüüüße

Don06 meinte damit wohl das eine mögliche Selbstzuweisung aktuell nicht behandelt wird.

Edit:
Was mir nach auffiel

o array(u32 size) sollte explizit sein
o Vergleichsoperatoren halte ich für unnötig
o Die Const-Correctness wurde nicht konsequent eingehalten (z.B. size())
o Die "Wachstumsstrategie" ist weniger optimal (viel zu viel Allokationen)
o Kein Kopierkonstruktor
o Kein korrektes Exceptionhandling, gerade bei Neuanforderung von Speicher. Das kann schnell zu ungültigen Zeigern und haufwenweisen Problemen führen.

LukasBanana

Abgesehn von der Definition des "iterator"s und des "const_iterator"s sieht das jetzt aber schon relativ gut aus, oder?!

zeusosc

David_pb schrieb:

zeusosc schrieb:
@Don06
array<T>& operator = (const array<T> &other) // Selbstzuweisung?
alternative??

Grüüüße
Don06 meinte damit wohl das eine mögliche Selbstzuweisung aktuell nicht behandelt wird.

Edit:
Was mir nach auffiel

o array(u32 size) sollte explizit sein
o Vergleichsoperatoren halte ich für unnötig
o Die Const-Correctness wurde nicht konsequent eingehalten (z.B. size())
o Die "Wachstumsstrategie" ist weniger optimal (viel zu viel Allokationen)
o Kein Kopierkonstruktor
o Kein korrektes Exceptionhandling, gerade bei Neuanforderung von Speicher. Das kann schnell zu ungültigen Zeigern und haufwenweisen Problemen führen.

ok,..
Wie behandelt man korrekterweise eine Selbstzuweisung??
Ich benutze eine ähnliche "Wachsutmsstrategie". Wie kann man das vermeiden?? (ausser nutzung std::container á la vector)
Wie sieht eine korrekte Fehlerbehandlung bei neuanforderung von speicher aus, ausser das das System sagt "hmmm, nö!".
*abo*
grüüße