C++ Gültigkeit von Variablen/Objekten

[te one]

Hallo,
ich entwickle seit einigen wenigen Wochen/Monaten des öfteren in C++ und habe eine recht grundlegende Frage. Ich versuche das mal anhand eines kleinen Beispiels (eher Pseudocode, Syntax mag eventuell im Detail falsch sein) zu verdeutlichen:

Tree t(...); // Tree is one of my own created classes
vector<Tree> reducedTrees;
for (unsigned int i=0; i<10; i++) { // reduce tree 10 times
    Tree reducedTree = t.reduce(i); // i is used as reduction parameter
    reducedTrees.push_back(reducedTree);
}
// Here I want to have 10 differently reduced trees inside the vector reducedTrees.

Wäre das so korrekt?
Ich debugge derzeit an so einem Stück Code und habe Angst, dass das Element, das im vector gespeichert ist nach einem for-Schleifendurchlauf ungültig wird.
Ich denke, dass ich eigentlich eine Referenz zum reducedTree in den vector lege, dieser reducedTree aber nur innerhalb der for-Schleife gültig ist und damit Teile des reducedTrees später unerwünscht überschrieben werden?!
Vielleicht hier jemand auflösen, ob da wirklich ein Problem entsteht und wie man das am saubersten löst?

Das Problem (ist es noch das selbe?) scheint mir hier deutlicher:

Tree t(...); // Tree is one of my own created classes
vector<int*> reducedTrees;
for (unsigned int i=0; i<10; i++) { // reduce tree 10 times
    Tree reducedTree = t.reduce(i); // i is used as reduction parameter
    int arr[reducedTree.size()];
    for (unsigned int j=0; j<reducedTree.size(); j++) {
        arr[j] = reducedTree.getElementById(j);
    }
    reducedTrees.push_back(arr);
}
// Here I want to have 10 differently reduced trees inside the vector reducedTrees.

Hier habe ich ja defintiv nur einen Pointer zum ersten Arrayelement in den vector gelegt. Wird das Array durch späteren Code eventuell überschrieben? Ist das das gleiche Problem wie oben?

Danke schonmal, glaube das ist wirklich eine Basic-Frage, die mich an mehreren Stellen komplett durcheinander bringt...

 

[new Account()]

Wäre das so korrekt?

Jup.

Ich denke, dass ich eigentlich eine Referenz zum reducedTree in den vector lege, dieser reducedTree aber nur innerhalb der for-Schleife gültig ist und damit Teile des reducedTrees später unerwünscht überschrieben werden?!

Du verwendest keine Referenzen oder Pointer, also wird die Kopie in den Vektor gelegt.

Hier habe ich ja defintiv nur einen Pointer zum ersten Arrayelement in den vector gelegt. Wird das Array durch späteren Code eventuell überschrieben? Ist das das gleiche Problem wie oben?

Nein, da du jeden Schleifendurchlauf ein neues array anlegen würdest, und sich somit der Pointer unterscheidet.
Scheint sich speziell mit Arrays anders zu verhalten: https://www.physicsforums.com/threads/c-declaring-an-array-within-a-loop.623897/
Ich sehe schon, warum ich raw arrays meide
Sollte dann wohl sowas sein: https://stackoverflow.com/questions...n-c-which-is-on-the-heap-instead-of-the-stack (Heap ALlokierung)
Es wäre aber sinnvoller auch hier einen Vektor zu nutzen (weil der Code ansonsten Speicherlecks hat).

Dir ist aber hoffentlich klar, dass du unten average(reducedTree.size())*10 (statt oben 10) Bäume drin hast?

 

[BeBur]

Du verwendest keine Referenzen oder Pointer, also wird die Kopie in den Vektor gelegt.

Das er einen Tree erstellt, dann davon eine flache Kopie in den Vektor schreibt und dann die erste Instanz direkt löscht ist aber doch semantisch vermutlich nicht korrekt.

 

[new Account()]

Das er einen Tree erstellt, dann davon eine flache Kopie in den Vektor schreibt und dann die erste Instanz direkt löscht ist aber doch semantisch vermutlich nicht korrekt.

Wenn das der Fall wäre (indem Tree einen owning Pointer enthalten würde), würde man nicht den Copy-Constructor überladen?

http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c61-a-copy-operation-should-copy

 

[Revan1710]

Also das zweite Beispiel halte ich für sehr problematisch: Du speicherst in deinem Vektor lediglich einen Pointer - in deinem Fall auf ein Int-Array. Wenn du nun später wieder über diesen Pointer auf das Array zugreifen willst, dann kann an der Stelle der Array-Elemente im Speicher schon wieder etwas völlig anderes drinstehen, denn die Lebenszeit deines Arrays arr endet mit dem Ende der For-Schleife.

Den ersten Fall halte ich auch für korrekt, wobei man sich das lokale Objekt wohl aber sparen könnte:

for (unsigned int i=0; i<10; i++) {
    reducedTrees.push_back(t.reduce(i));
}

 

[Grimba]

Ich bin mir nicht so ganz sicher, ob der erste Fall korrekt ist, denn das Object reducedTree wird ja hier knallhart auf dem Stack erzeugt, und lebt damit nur solange, wie der Scope, in dem es sich befindet. push_back macht zwar eine Copy, aber laut
https://stackoverflow.com/questions/2275076/is-stdvector-copying-the-objects-with-a-push-back
geht dir am Ende des Scopes das Objekt zur Referenz flöten. Damit würdest du dann einen Vector mit ungültigen Einträgen erzeugen. Also für mein Empfinden ist das ne böse Nummer. Gleiches gilt für das zweite Beispiel, da stimme ich meinem Vorredner zu.

Ich würde sagen, du müsstest hier mit new arbeiten.

 

[new Account()]

aber laut
https://stackoverflow.com/questions/2275076/is-stdvector-copying-the-objects-with-a-push-back
geht dir am Ende des Scopes das Objekt zur Referenz flöten. Damit würdest du dann einen Vector mit ungültigen Einträgen erzeugen.

Es wird in den Vektor reinkopiert (d.h. an einen anderen Speicherort kopiert (hier: im Heap)), und dann wird das kopierte Objekt ungültig.
Passt doch alles.

 

[Grimba]

Ja, das ist so einer dieser Fälle, wo ich mir Java zurückwünsche
Keine Ahnung, mir stellen sich bei sowas gefühlt die Nackenhaare hoch. Aber es gibt gute Gründe, "new" zu meiden, da teuer.

 

[new Account()]

Aber es gibt gute Gründe, "new" zu meiden, da teuer.

Nicht nur deswegen:
http://isocpp.github.io/CppCoreGuid...-delete-outside-resource-management-functions

 

[Grimba]

Ja gut, dass man aufpassen muss, wann was noch lebt, hab ich hier vorausgesetzt, da braucht man nicht extra drüber reden. Aber im Vergleich zu Java, wenn ich den schon anbringe, ist das natürlich eine wichtige Ergänzung, aber die meinte ich jetzt nicht.

 

[BeBur]

imHo die korrekte Umsetzung wäre mittels unique_ptr im vektor.
Raw Pointer verwendet man ohne guten Grund doch gar nicht mehr seit C++11, also bald 10 Jahre.

 

[new Account()]

imHo die korrekte Umsetzung wäre mittels unique_ptr im vektor.

Was spräche denn gegen einen Vektor?

 

[BeBur]

Ich glaube nichts, allerdings wäre hier dann vermutlich emplace_back statt push_back semantisch angebracht. Damit sollte die Erstellung der Kope vermieden werden.

 

[te one]

So, ich bin erstmal erfreut, dass die Antwort wohl doch nicht ganz so einfach war. Vielen Dank für die vielen Tipps. Erstere Lösung finde ich sowieso schöner. Da es leider noch nicht läuft, möchte ich hier mal etwas mehr Code zeigen (etwas aufgeräumt, aber alles wichtige sollte drin sein):

vector<int> neededReductions; // Usually this vector is filled with data by the user (command line). We use 10 and 20 as an example.
neededReductions.push_back(10);
neededReductions.push_back(20);

// The following vectors of maps have the structure:
// The outer vector holds data for different scans - for this example we only need one scan
// The inner map maps a reduction parameter (here 10 or 20) to some data
// Data could be xyz data of 3D points or the semantic class of these points
vector <map <int, DataXYZ>> reducedPoints;
vector <map <int, vector<int>>> reducedTypes; // This was of type vector <map <int, DataType>> but I had trouble accessing it later (read strange values)...
// For each 3d scan (each consisting of around 20.000.000 3d points)
for(ScanVector::iterator it = Scan::allScans.begin(); it != Scan::allScans.end(); ++it) {
    map <int, DataXYZ> reducedPointsMap;
    map <int, vector<int>> reducedTypesMap;
    Scan* scan = *it;
    // Reduce the scan in different scales (here: 10=only a bit of reduction, 20=much reduction)
    for(unsigned int i = 0; i < neededReductions.size(); i++) {
        scan->setReductionParameter(neededReductions[i], ...);
        scan->calcReducedPoints();
        DataXYZ xyz(scan->get("xyz reduced")); // This is now a reduced scan (xyz data)
        reducedPointsMap.insert({neededReductions[i],xyz});
        DataType dataType = scan->get("type reduced"); // This is a "class" for every point in the reduced scan
        // I wanted to put this DataType object into the map reducedTypesMap - but I hat trouble accessing it later... So now using vector of ints
        vector<int> typesOfThisReduction;
        for (unsigned int typeId = 0; typeId < dataType.size(); typeId++) {
            typesOfThisReduction.push_back(dataType[typeId]);
        }
        reducedTypesMap.insert({neededReductions[i],typesOfThisReduction});
    }
    reducedPoints.push_back(reducedPointsMap);
    reducedTypes.push_back(reducedTypesMap);
}
cout << reducedPoints.at(0).find(10)->second.size() << endl; // Show me size of first scan (0) with reduction of 10. This works great :)
cout << reducedPoints.at(0).find(10)->second[0][0] << endl; // BOOM! Program crashes (depending on input and reduction parameters. E.g.: My one input file works with reductions 10 and 11, but not with 10 and 20 as shown in this example.
// Later I want to do:
// vector <map <int, KDtree*>> reducedKDtrees;
// Foreach scan, foreach reduction, foreach point j:
//        vector<double*> points;
//        points.push_back(new double[3]{it->second[j][0], it->second[j][1], it->second[j][2]}); // This is the way other people used before to create a KDtree
//        KDtree* t = new KDtree(&points[0], points.size()); // I used new-Keyword for my KDtree. I didnt want to (want high performance) but also had problems with it on the stack...
//        Put that all into reducedKDtrees

Wie man in den Kommentaren sieht, konvertiere ich mittlerweile schon "DataType" in einen vector<int> um, da ich sonst später falsche Werte ausgelesen habe... Gerne würde ich das auch wieder rückbauen. Aktuell crasht es noch, da wohl auch "DataXYZ" nicht konsistent gehalten wird.
Das ganze läuft innerhalb eines großen Projektes.
DataXYZ bzw. DataType sind zweimal definiert, bin mir gerade unsicher welche Definition verwendet wird (dürften aber gleich sein):
https://sourceforge.net/p/slam6d/code/HEAD/tree/trunk/include/slam6d/data_types.h
https://sourceforge.net/p/slam6d/code/HEAD/tree/trunk/include/scanserver/multiArray.h
Und hier der KDtree (bis dahin läuft mein Programm derzeit aber ja nicht):
https://sourceforge.net/p/slam6d/code/HEAD/tree/trunk/include/slam6d/kd.h
https://sourceforge.net/p/slam6d/code/HEAD/tree/trunk/src/slam6d/kd.cc

Vielen Dank bereits.
Ich hoffe, ihr könnt mich noch zum Ziel führen

 

[te one]

Neues:
Ich habe es nun erstmal so gefixt, dass ich in der innersten Loop, wo ich mir die DataXYZ-Objekte hole (sind eigentlich nur ein Zwischenprodukt) auch dort direkt die jeweiligen KDtree* mit new erzeuge und befülle. Das klappt soweit. Wollte es eigentlich wegen späterem Multithreading auseinander ziehen - aber nun ist es wohl so...

Wenn jemand weiß warum mir der obige Code um die Ohren fliegt (bei Zeile 34 passiert der große Knall) wäre ich euch jedoch sehr verbunden

Frohe Weihnachtszeit

 

[BlackMark]

Mach mal aus Zeile 20 und 21 ein:

reducedPointsMap.insert({neededReductions[i], DataXYZ(scan->get("xyz reduced"))});

Funktioniert das?

Edit:
Wahrscheinlich nicht. Mach zusätzlich aus Zeile 30:

reducedPoints.push_back(std::move(reducedPointsMap));

Funktioniert es so?

Gruß
BlackMark

 

[te one]

Funktioniert es so?

Leider nein...

Und soeben habe ich mal die Inhalte von

vector <map <int, vector<int>>> reducedTypes;

genauer gecheckt... Passen leider auch nicht. Ich schreibe die richtigen Daten rein (Zahlen von 0 bis 9), lese aber später andere Werte aus (zB 1.701.869.940). Das ganze habe ich in meinen Tests nicht gesehen. Mit meinen Echtdaten ging das jetzt die ersten ca. 162.700 Datenpunkte gut - ab dann ist das meiste Schrott :/ Denke also, dass ab da überschrieben wurde.

edit: Nebenfrage: Wenn ich nun Elemente mit new erzeuge, die ich während des ganzen Programmablaufs benötige... Kann ich die dann einfach stehen lassen oder sollte ich die auch am Ende löschen?

 

[new Account()]

vector <map <int, TripleArray<double>>> reducedPoints;

reducedPoints.at(0) // Typ: map <int, TripleArray<double> >
reducedPoints.at(0).find(10)->second // Typ: TripleArray<double> <
reducedPoints.at(0).find(10)->second[0] // Typ: double*
reducedPoints.at(0).find(10)->second[0][0] // Typ: double

Wir wissen doch garnicht was in DataXYZ drin ist, woher sollen wir also wissen was am Code falsch läuft?
EDIT: ... sehe gerade den Link... schaue mal drüber.
EDIT: Benutzt du hier den Indexoperator auf einem double? Oder habe ich etwas falsch interpretiert?
EDIT: ja, habe ich: es wird ein double* zurückgegeben, kein double -> Scheint ein Problem im TripleArray zu sein.
Hast du mal versucht beim reinkopieren in die Vektoren die DataXYZ alle auszuprinten?
EDIT: Hast du mal geschaut, ob DataXYZ noch irgendwo rauskopiert wird vorher aus dem Vektor?
Wenn ja, könnte da das Problem liegen: Im Code hat der Copy Operator move semantics implementiert. Weird und gefährlich imho. Das würde afaik nämlich DataXYZ ungültig machen ohne std::move zu nutzen.

  inline void  shallowCopy(const DataPointer& other) {
    if(m_private_impl != 0)
      delete m_private_impl; //// Wieso? wird dadurch nicht der Speicher gelöscht, sodass er auch in der Kopie nicht mehr verfügbar ist?
    m_pointer = other.m_pointer;
    m_size = other.m_size;

    m_private_impl = other.m_private_impl;
  }


    //copy assignment operator moves the object
        inline TripleArray& operator=(const TripleArray & other)
      {
        if(&other!=this)
      {
            //TripleArray temp(other);
        //std::swap(*this,temp);
        this->shallowCopy(other);
      }
        return *this;
      }

Allgemein wäre ein funktionierender minimaler Beispielcode hilfreich, dann wäre das schnell gelöst.

Übrigens könntest du deine Schleifen durch for(const auto& item: items){} ersetzen. Das würde den Code einfacher und wengier fehleranfällig machen.

Drittens, KDtree* t = new KDtree(&points[0], points.size()); zu std::unique_ptr<KDtree>=std::make_unique(&points[0], points.size()) ändern.

 

[Grimba]

edit: Nebenfrage: Wenn ich nun Elemente mit new erzeuge, die ich während des ganzen Programmablaufs benötige... Kann ich die dann einfach stehen lassen oder sollte ich die auch am Ende löschen?

Mit new erzeugst du auf dem Heap das Objekt und auf dem Stack einen Zeiger dazu.

Sofern nichts explizit das Objekt löscht, bleibt es da bis zum Programmende.

Das ist aber ein zweischneidiges Schwert, denn mitunter stirbt der Zeiger auf das Objekt irgendwo im Programm, und dann bleibt das Objekt im Speicher, du kannst es aber nicht mehr finden, wenn das der letzte Zeiger war. Du solltest also sehr genau darauf achten, wann und wie viele solcher Objekte du so erzeugt hast, und wie viele Zeiger es darauf gibt. Am besten verpackt man Pointer auf solche Objekte in Smartpointer, die den Überblick über alle referenzierenden Zeiger auf ein Objekt behalten, und dann das Objekt, sobald der letzte Zeiger stirbt, löschen.

Also wenn du davon ausgehst, dass du die Objekte bis in alle Ewigkeit bis zum Programmende benutzt, kannst du das theoretisch schon so machen, wenn du immer auf deine Pointer achtest, denn mit Programmende wird der Speicher auch wieder freigegeben vom OS. Aber es ist unsauber.

 

[te one]

Allgemein wäre ein funktionierender minimaler Beispielcode hilfreich, dann wäre das schnell gelöst.

Übrigens könntest du deine Schleifen durch for(const auto& item: items){} ersetzen. Das würde den Code einfacher und wengier fehleranfällig machen.

Drittens, KDtree* t = new KDtree(&points[0], points.size()); zu std::unique_ptr<KDtree>=std::make_unique(&points[0], points.size()) ändern.

Ja, komme wohl wirklich nicht drumherum mal ein Beispiel ohne externen Code zu bauen. Ich setze mich mal dran.
Dass ich kein for(const auto...) verwende ist Absicht: Später läuft der Code multithreaded und ich möchte die entsprechende ID des Durchlaufs regelmäßig ausgeben. Pro 100.000 abgearbeitetenden Punkte meldet sich ein Thread und sagt, dass er gerade bei Scan Nr # ist, Reduction # und Punkt # behandelt. Damit kann man grob die Dauer abschätzen (meine vorherige Version hatte funktioniert und lief teilweise Tage).
Okay, das mit unique_ptr teste ich mal Aber erstmal baue ich Beispielcode.

Also wenn du davon ausgehst, dass du die Objekte bis in alle Ewigkeit bis zum Programmende benutzt, kannst du das theoretisch schon so machen, wenn du immer auf deine Pointer achtest, denn mit Programmende wird der Speicher auch wieder freigegeben vom OS. Aber es ist unsauber.

Super. Ja, das hier ist mehr ein Skript, dass man auf bestimmte Daten loslässt. Die erzeugten Bäume brauche ich über die ganze Skriptlaufzeit.