Ist Iterieren über Elemente, die gerade gelöscht werden UB?

Mir fliegt gerade mein Code um die Ohren und ich habe folgendes Konstrukt im Verdacht:

#include <vector>
#include <iostream>
#include <memory>

struct Foo;

std::vector<std::unique_ptr<Foo>> v;

struct Foo{
	void print(){
		std::cout << this;
	}
	~Foo(){
		for (auto &e : v){ //iterieren über Elemente, die gerade gelöscht werden
			e->print();
		}
	}
};

int main(){
	v.emplace_back(std::make_unique<Foo>());
	v.emplace_back(std::make_unique<Foo>());
	v.emplace_back(std::make_unique<Foo>());
	v.clear();
}

UB oder OK?

Er greift in der for-Schleife auf gelöschte Objekte zu, deswegen crasht es

Wenn man

v.clear();

durch

while (v.size())
    v.pop_back();

ersetzt, dann wird das Problem umgangen. Ausgesprochen unschön.

Ist doch klar, was passiert: Du greifst auf destructed unique_ptrs zu.

v.emplace_back(std::make_unique<Foo>());
    v.emplace_back(std::make_unique<Foo>());
    v.emplace_back(std::make_unique<Foo>());
    v.clear();

/* Erstes Foo löschen:
   | v[0].~unique_ptr();
   | | v[0]->print(); // ok, weil v[0] noch nicht deleted hat
   | | v[1]->print();
   | | v[2]->print();
   | | delete v[0].get(); // Speicher freigeben

   Zweites Foo löschen:
   | v[1].~unique_ptr();
   | | v[0]->print(); // bäm
   | | v[1]->print();
   | | v[2]->print();
   | | delete v[1].get(); // Speicher freigeben

   Drittes Foo löschen:
   | v[2].~unique_ptr();
   | | v[0]->print(); // bäm
   | | v[1]->print(); // bäm
   | | v[2]->print();
   | | delete v[2].get(); // Speicher freigeben
*/

Bei deinem zweiten Code löschst du die unique_ptrs, bei denen der Destruktor aufgerufen wurde aus dem Vector raus, deshalb führt es zu keinen Problemen bei der for-Loop.

Dein Code ist allgemein

nwp3 schrieb:

Ausgesprochen unschön.

wofür braucht man den so etwas

waspassiert schrieb:

wofür braucht man den so etwas

GUI-Kram. Ich habe eine Art Radio-Button der gelöscht werden kann und wenn er ausgewählt war muss er einen anderen Radio-Button auswählen, damit die Liste keine leere Auswahl hat. Und wenn ein anderes Set von Radio-Buttons ausgewählt wird muss die ganze Liste gelöscht werden und die Buttons schieben sich gegenseitig den "ausgewählt"-Status zu.

Alternativen wären
-ein "Ihr werdet gleich alle gelöscht also wählt keinen Nachfolger"-Flag setzen und abfragen
-manuell den aktiven Button inaktiv setzen, dann wird nichts weitergeschoben
-eine leere Auswahl zulassen
-Proposal für C++17 machen das vorschreibt dass das funktionieren muss und solange warten

Nichts davon sieht sonderlich überzeugend aus.

RAII in Ehren, aber einen Nachfolger zu bestimmen ist keine Aufgabe für einen Destruktor ...

Jemand wird den Button initialisieren und ihm den richtigen Zustand zuweisen. Genau derjenige sollte also ihn wieder löschen. Symmetrie.

Das was du machst ist ein delete-this-Idiom. Sowas führt mittelfristig immer zu Problemen...

Zerlöscherer schrieb:

Jemand wird den Button initialisieren und ihm den richtigen Zustand zuweisen. Genau derjenige sollte also ihn wieder löschen. Symmetrie.

Habe ich doch. Dieselbe Funktion, die die Buttons in den vector stopft ruft auch vector.clear() auf. Den Zustand bekommen sie im Konstruktor und verlieren ihn im Destruktor, alles schön symmetrisch.

waspassiert schrieb:

RAII in Ehren, aber einen Nachfolger zu bestimmen ist keine Aufgabe für einen Destruktor ...

Wahrscheinlich ist das die richtige Lösung. Statt direkt die Buttons zu löschen erst die Auswahl ändern und beim clear eben nicht.

hustbaer

Die pop_back Löschen garantiert auch net dass es klappen muss. Auch dabei würde der Destruktor deiner Buttons laufen während eine vector Memberfunktion (nämlich pop_back ) noch ausgeführt wird. Und über den Umweg eines Destruktors auf eine Collection zuzugreifen die gerade modifiziert wird ist UB. Behaupte ich mal. Würde mich SEHR wundern wenn nicht.

Lösung:

void RemoveLastOne()
{
    unique_ptr<Foo> limbo;
    std::swap(v.back(), limbo);
    v.pop_back();
} // ~Foo läuft jetzt hier, nachdem pop_back fertig gelaufen ist => jetzt alles OK

Und wenn du alle löschen willst, dann kannst du natürlich auch gleich

void RemoveAll()
{
    vector<unique_ptr<Foo>> limbo;
    std::swap(v, limbo);
}

machen.

Meine GUI Library verwendet aus diesem und ähnlichen Gründen eine eigene "ChildControlCollection". Die kümmert sich um solche Details, so dass man im Code für die einzelnen Controls bzw. Client Code nicht auf sowas aufpassen muss.