Unerklärliches Problem beim Iterieren über std::list

[krümelkacker]

branleb schrieb:

template <typename string_type> class Log
    {
      public:
        typedef Logger<string_type> logger_type; 
        typedef std::list<logger_type*> logger_list_type;
 
      private:
        Log()
        {
          this->loggers = new std::list<logger_type*>();
        }

        logger_list_type* loggers;

      public:
        void operator()(const string_type & message) const
        {
          for(logger_list_type::iterator iter = this->loggers->begin(); iter != this->loggers->end(); iter++) {
            iter->logMessage(message);
          }
        }
    };

Gibt es einen besonderen Grund, warum Du die Liste per new angelegt hast?
(Tipp: Die Antwort ist "Nein. Ich komme von Java und weiß es nicht besser.")

kk

[drakon]

branleb schrieb:

Hat mir zufällig jemand einen Link oder eine kurze Erklärung,
warum das hier nötig ist?

Ganz kurze Erklärung: Der Compiler kann nicht wissen zu was logger_list_type::iterator aufgelöst wird. Es kann ein Typ oder z.B eine Funktion in einem Namespace sein. Darum musst du ihm mitteilen, dass es ein Typ ist.

[branleb]

krümelkacker schrieb:

branleb schrieb:

template <typename string_type> class Log
    {
      public:
        typedef Logger<string_type> logger_type; 
        typedef std::list<logger_type*> logger_list_type;
 
      private:
        Log()
        {
          this->loggers = new std::list<logger_type*>();
        }

        logger_list_type* loggers;

      public:
        void operator()(const string_type & message) const
        {
          for(logger_list_type::iterator iter = this->loggers->begin(); iter != this->loggers->end(); iter++) {
            iter->logMessage(message);
          }
        }
    };

Gibt es einen besonderen Grund, warum Du die Liste per new angelegt hast?
(Tipp: Die Antwort ist "Nein. Ich komme von Java und weiß es nicht besser.")

kk

Ähh - nein. Ich komme nicht aus Java. Aber ja, ich weiß es ernsthaft nicht besser. Wie hätte ich sie anlegen sollen?

[drakon]

Die wird ganz automatisch im Konstruktor initialisiert.

Wenn du ein solches Objekt innerhalb einer Funktion definierst, dann machst du das ja auch so:

std::list<int> my_list;

Da brauchst du nichts weiteres. Du kannst natürlich in der Initialisierungsliste noch Konstruktorparameter übergeben, wenn du magst, aber ansonsten wird der Standardkonstruktor aufgerufen.

[SeppJ]

branleb schrieb:

Ähh - nein. Ich komme nicht aus Java. Aber ja, ich weiß es ernsthaft nicht besser. Wie hätte ich sie anlegen sollen?

Einfach als normaler Member:

template <typename string_type> class Log
    {
     // ...
      private:
        logger_list_type loggers;

    // ...
    };

[Nexus]

Das ist auch nur eine von vielen Unschönheiten in deinem Code. Verbesserungsvorschläge als Kommentar:

template <typename string_type> class Log 
    {
      // Warum public? Die Typen kommen nirgends in der Schnittstelle vor 
      public: 
        typedef Logger<string_type> logger_type; 

        // Nimm std::list<logger_type>
        typedef std::list<logger_type*> logger_list_type; 

      private:
        // Somit kann niemand ein Log-Objekt erstellen und benutzen
        // Mach den Konstruktor public oder schränke Zugriff spezifisch ein
        Log() 
        { 
          // Prinzipiell dienen Typedefs dazu, benutzt zu werden
          // Da du jetzt aber kein new verwendest, must du nichts tun
          // (höchstens ein loggers() in der Initialisierungsliste)
          this->loggers = new std::list<logger_type*>(); 
        } 

        // Nimm logger_list_type
        logger_list_type* loggers; 

      public: 
        void operator()(const string_type & message) const 
        { 
          // typename logger_list_type::iterator
          // this ist unnötig und macht den bereits langen For-Header noch unübersichtlicher
          // ++iter statt iter++ spart unnötige Kopie
          // evtl. noch end() speichern statt jedes Mal zu prüfen
          for(logger_list_type::iterator iter = this->loggers->begin(); iter != this->loggers->end(); iter++) { 
            iter->logMessage(message); 
          } 
        } 
    };

[branleb]

Nexus schrieb:

Das ist auch nur eine von vielen Unschönheiten in deinem Code.

Spricht wirklich was dagegen, die Liste auf dem Heap anzulegen?

Nexus schrieb:

Verbesserungsvorschläge als Kommentar:

template <typename string_type> class Log 
    {
      // Warum public? Die Typen kommen nirgends in der Schnittstelle vor 
      public: 
        typedef Logger<string_type> logger_type; 

        // Nimm std::list<logger_type>
        typedef std::list<logger_type*> logger_list_type;

hier nicht, aber der Code war auch nur ein auschnitt.
logger type kam im interface drin vor.

Für logger_list_type hast du wohl recht, was das public angeht.
Ich hatte aber ansonsten meine Gründe, Zeiger zu nehmen.
Eventuell könnte ich das ganze auch über Referenzen machen,
dass müsste ich erst noch prüfen.

Nexus schrieb:

private:
        // Somit kann niemand ein Log-Objekt erstellen und benutzen
        // Mach den Konstruktor public oder schränke Zugriff spezifisch ein
        Log() 
        { 
          // Prinzipiell dienen Typedefs dazu, benutzt zu werden
          // Da du jetzt aber kein new verwendest, must du nichts tun
          // (höchstens ein loggers() in der Initialisierungsliste)
          this->loggers = new std::list<logger_type*>(); 
        }

Absicht, die Klasse Log stellt ein Singleton dar,
die getInstance Methode hatte ich weggelassgen,
weil sie für das Problem nicht relevant var.
Es war eben nur ein Auschitt der Klasse.

Nexus schrieb:

// Nimm logger_list_type
        logger_list_type* loggers;

Was spricht den dagegen, die Logger objekte auf dem Heap anzulegen und folglich eben einen Zeiger und new zu nehmen?

Nexus schrieb:

public: 
        void operator()(const string_type & message) const 
        { 
          // typename logger_list_type::iterator

Okay, das ist inzwischen klar warum, hab ich auch gemacht.

Nexus schrieb:

// this ist unnötig und macht den bereits langen For-Header noch unübersichtlicher

Ich habe mir this angwöhnt, um zu sichern, dass es um eine objektvariable geht.

Nexus schrieb:

// ++iter statt iter++ spart unnötige Kopie

Dann überspringe ich aber den ersten Logger oder?

Nexus schrieb:

// evtl. noch end() speichern statt jedes Mal zu prüfen
          for(logger_list_type::iterator iter = this->loggers->begin(); iter != this->loggers->end(); iter++) { 
            iter->logMessage(message); 
          } 
        } 
    };

Ob end() zwischenspeichern so viel bringt?

Oder sind meine Einwände nicht berechtigt?
Ich bin gern bereit zu lernen,
aber ich würde gerne wissen, warum meine Gedanken dazu nicht richtig sind…

Gruß,
Branleb

[Nexus]

branleb schrieb:

Spricht wirklich was dagegen, die Liste auf dem Heap anzulegen?

Wichtig ist die Unterscheidung zwischen dynamischer und automatischer Speicherverwaltung, nicht Heap und Stack. Gegenfrage: Spricht etwas gegen eine automatische Variable?

branleb schrieb:

Ich hatte aber ansonsten meine Gründe, Zeiger zu nehmen.

Was für Gründe? Manchmal sind Zeiger schon sinnvoll.

branleb schrieb:

Eventuell könnte ich das ganze auch über Referenzen machen

Nein.

branleb schrieb:

Was spricht den dagegen, die Logger objekte auf dem Heap anzulegen und folglich eben einen Zeiger und new zu nehmen?

Manuelle Speicherverwaltung und ihre Probleme, Geschwindigkeits- und Speicheroverhead, zusätzliche Indirektion. Und was spricht für dynamische Allokation?

branleb schrieb:

Dann überspringe ich aber den ersten Logger oder?

Nein. Schau dir nochmals die Bedeutung von Prä- und Postfixinkrement an.

branleb schrieb:

Ob end() zwischenspeichern so viel bringt?

Hängt von der Implementierung deiner Standardbibliothek ab. Wahrscheinlich wird es nicht viel bringen, darum "evtl.".

[drakon]

branleb schrieb:

Nexus schrieb:

Das ist auch nur eine von vielen Unschönheiten in deinem Code.

Spricht wirklich was dagegen, die Liste auf dem Heap anzulegen?

Was spricht denn dafür?

Dagegen spricht, dass du dich um den Speicher selbst kümmern musst, die Exceptionsicherheit komplizierter umzusetzen macht und es völlig unnötig ist.

Prinzipiell sollten Variablen automatisch (ohne new; auf dem Stack) angelegt werden und nur wenn es nötig ist dynamisch. Warum? - Weil es einiges simpler zu handhaben, robuster und wahrscheinlich auch schneller ist.

Dann überspringe ich aber den ersten Logger oder?

Nein. Da wird einfach das Objekt inkrementiert und das neue zurückgegeben. Bei Postinkrement wird das alte Objekt zurückgegeben. Hier benutzt du den Rückgabewert sowieso nicht, also kannst du dir das auch gleich sparen.

Ob end() zwischenspeichern so viel bringt?

Sehr viel wahrscheinilch nicht, aber es bringt ein wenig was und es macht den Code auch ein wenig lesbarer.

~//EDIT: 4 Sekunden und wiedermal fast gleiche Antworten. :P~

[Nexus]

drakon schrieb:

Sehr viel wahrscheinilch nicht, aber es bringt ein wenig was und es macht den Code auch ein wenig lesbarer.

Nein, es macht ihn weniger lesbar.

for (Iterator itr = c.begin(); itr != c.end(); ++itr)
// vs
for (Iterator itr = c.begin(), end = c.end(); itr != end; ++itr)

Mehrere Zeilen will ich nicht verwenden. Aber ich hab mir dafür ein Makro gebastelt, wenn ich mal nicht Boost.Foreach benutzen kann. Und auch wenn ich es benutzen kann, habe ich bei "ein paar Prozent" Performance-Einbussen von BOOST_FOREACH ein mulmiges Gefühl. Bisher war es zum Glück noch nie das Performance-Problem.

P.S.: Ja, die ähnlichen Antworten... Mit dir ist es immer das Gleiche.

[drakon]

Ich dachte dabei eher an so etwas:

logger_list_type::iterator begin = this->loggers->begin();
logger_list_type::iterator end = this->loggers->end();
for(logger_list_type::iterator iter = begin; iter != end; ++iter)
{
  iter->logMessage(message);
}

Finde ich persönlich besser lesbar. Es ist zwar mehr Text, aber bei sowas sehe ich sehr viel schneller was abgeht, als wenn ich zuerst wirklich schauen muss was im Schleifen Kopf steht.

Aber es ist natürlich ein zu grosser Aufwand für eine solche banale Sache. Ich erwarte sehnlichst das hier:

for(auto iter: this->loggers){
  iter->logMessage(message);
}

[Nexus]

Ich mag kleinen Code. Besonders bei Standardaufgaben wie Container-Iteration bin ich froh, nicht viel Boilerplate-Code schreiben und lesen zu müssen.

MY_FOREACH(Container, c, itr)

ist mir so viel lieber als

Container::iterator begin = c.begin();
Container::iterator end = c.end();
for (Container::iterator itr = begin; itr != end; ++itr)

Mal davon abgesehen, dass du unnötige Variablen erzeugst und den umliegenden Scope verschmutzt.

[drakon]

^^
Ich habe nie das Gegenteil behauptet. Ich lass es meist drin und formatier es entsprechend, dann gehts auch mit der Lesbarkeit.

Ein Makro wäre eine Idee, aber irgendwie war das Bedürfnis noch nicht gross genug, dass ich mir da was zusammengebastelt habe.