Was haltet ihr von meiner eigenen CONTAINER Klasse?

camper

LukasBanana schrieb:

Sie müsste mehr oder weniger Fehlerfrei sein.

Ist sie bei weitem nicht, was durch minimale Tests feststellbar ist. Hausaufgaben machen wir hier nicht. Falls du eine spezifische Frage hast, stelle sie.
Es erschließt sich mir bisher nicht, zu welchem Zweck dieser Thread eröffnet wurde.

Maxi

na ich sehe jetzt zwar nicht so den Sinn darin, aber das hier ist mir aufgefallen:

delete [] array_.
add sollte const T& als parameter nehmen, oder noch besser abhängig obs kleine oder große Typen sind (boost hat da was zu)
Dein Container funzt wohl nicht richtig mit komplexen Objekten, da du den Speicehr mit memcpy koopierst. Verwende lieber std::copy
swap tauscht den Inhalt nicht, wenn ein Container leer ist
op== sollte auf != zugreifen oder andersrum. außerdem sind zwei container doch gleich, wenn beide leer sind, oder?
du solltest einen const T& op[] const mit anbieten.
flip und unique sollte lieber außerhalb implmenetiert werden, da diese algos auch auf anderen strukturen funktionieren. Deshalb gibts ja auch std::reverse und std::unique

Don06

typedef T* const    const_iterator;

Das geht zwar, aber die std:: Container machen das anders.

Gruß
Don06

otze

sollte es nicht eh

typedef const T* const_iterator;

heissen?immerhin soll ja T konstant sein, und nicht der iterator.

camper

Kurz beim Überfliegen durchgezählt:
12 offensichtliche Fehler
12 Designfehler
18 WTFs
Was ich davon halte? gar nichts.

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?