delete [] thread-safe machen

daddy_felix

wenn der Code gut mit Unit Tests abgedeckt ist, sollte das Refactoring recht schnell gehen. Wenn diese aber nur unvollständig oder gar nciht vorhanden sind, dann wird es unangenehm...

hustbaer

Auch das ist Wunschdenken.
Kein nicht-trivialer Code ist soweit mit Unit-Tests abgesichert, dass diese 100% aller möglichen Fälle abdecken. Meist nichtmal 100% aller relevanten Fälle.

OK, man *kann* dann flott refactorn, nur kann man eben nicht sicher sein dass nachher auch wirklich alles passt, nur weil die Tests durchlaufen. Fehlerbehandlung ist z.B. etwas, was in kaum einem Projekt auch nur annähernd brauchbar von automatischen Tests abgedeckt ist.

cooky451 schrieb:

Denke ich auch. 3000 Zeilen sind in einer Stunde durch, da lohnt das Ärgern doch nicht.

Du hast noch keinen wirklich üblen Code übernehmen müssen. 3000 Zeilen können ausreichen, um einen Tage- oder Wochenlang zu beschäftigen. Neu schreiben ist manchmal auch nicht drin. Z.B. wenn dieser Code mit Hardware kommuniziert, aber nichts, nicht mal das Protokoll, dokumentiert ist. So einen haufen Müll zu verstehen dauert sehr lange. Und teilweise ist das alles noch so fragil, dass jede Änderung Adrenalinschübe verursacht.

Willkommen in der realen Welt

Und selbst bei wirklich guten Code* führt man keine größeren Änderungen in einer Stunde durch. Du hast Vorstellunge...

*Was der Notwendigkeit nach Überarbeitung natürlich widerspricht.

curry-king

Bei über 3000 Zeilen von folgendem Code würde das refactoring dann wahrscheinlich ein MJ dauern :-)...
Mein Problem ist gelöst, ich hatte mehr freigegeben als ich allokiert hatte!

int update_table2(vector<string> staterows, vector<string> transrows, vector<string> columns,bool*** table, string stream, size_t old_states, size_t old_trans, size_t old_columns)
{
	bool **new_table;

	(new_table) = new bool* [(columns).size()];
	for(unsigned int iu=0;iu<(columns).size();iu++) (new_table)[iu] = new bool[(staterows).size()+(transrows).size()];

	string newquerry("");
	for(unsigned int iu=0;iu<(staterows).size()+(transrows).size();iu++)
	{
		for(unsigned int ju=0;ju<(columns).size();ju++)
		{	
			if((iu<old_states)&&(ju<old_columns-1)) { // copy old states
			new_table[ju][iu] = (*table)[ju][iu]; 
			}
			else{
				if((iu>staterows.size()-1)&&(ju<old_columns-1)&&(iu<staterows.size()+old_trans)){ //copy old transitions
					new_table[ju][iu] = (*table)[ju][iu-staterows.size()+old_states]; 
				}
				else{
					newquerry = (iu>(staterows).size()-1)? (transrows)[iu-(staterows).size()] : (staterows)[iu];
					newquerry += (columns)[ju];
					(new_table)[ju][iu]=querry(newquerry,stream); //transitions
				}
			}
		}
	}

	// copy new_table into table
	delete [] (*table);
	(*table) = new bool* [(columns).size()];
	for(unsigned int iu=0;iu<(columns).size();iu++) (*table)[iu] = new bool[(staterows).size()+(transrows).size()];
	for(unsigned int iu=0;iu<(staterows).size()+(transrows).size();iu++)
	{
		for(unsigned int ju=0;ju<(columns).size();ju++)
		{	
			(*table)[ju][iu]=new_table[ju][iu]; //copy
		}
	}

	delete [] new_table;
	return 0;
}

bool fix_closed(vector<string> *staterows, vector<string> *transrows, vector<string> *columns, vector<string>alpha, bool ***table,string stream)
{
	size_t old_state = (*staterows).size();
	size_t old_trans = (*transrows).size();
	size_t old_col = (*columns).size();

	bool found;
	string transition;
	string help;
	for(unsigned int i=0;i<(*transrows).size();i++)
	{
		for(unsigned int j=0;j<(*staterows).size();j++)
		{
			found = true;
			for(unsigned int k=0;k<(*columns).size();k++)
			{
				if ((*table)[k][j] != (*table)[k][i+(*staterows).size()]) {found = false; break;}
			}
			if(found) break;
		}
		if (!found)
		{// fix transrow 
			transition = (*transrows)[i];
			(*staterows.......................................

Shiba

cooky451 schrieb:

3000 Zeilen sind in einer Stunde durch, da lohnt das Ärgern doch nicht.

Respekt
Erfassen, verstehen und eventuell korrigieren einer Zeile in 1,2 Sekunden.

Shiba schrieb:

Respekt
Erfassen, verstehen und eventuell korrigieren einer Zeile in 1,2 Sekunden.

Naja, es ist ja nicht jede Zeile betroffen. Eine Stunde ist natürlich dennoch viel zu wenig.

Aber bei dem geposteten Code ist dennoch durch minimale Änderungen einiges zu verbessern. Bspw. die Übergabe als Wert und damit das notwendige Kopieren könnte mit konstanten Referenzen leicht umgangen werden, ohne den Code zu ändern.
Und auch sonst wäre in diesem Beispiel wahrscheinlich der Einsatz von Pointer-Containern die bessere Wahl...

camper

Hier lässt sich sogar ohne stilistische Änderungen einiges verbessern.
Wer löscht eigentlich die alten bool-Arrays? Ich vermisse da ein delete[].
Kann nat. auch an anderer Stelle des Programmes stehen... in diesem Fall wäre es allerdings diese geteilte Zuständigkeit, um die ich mich zuerst kümmern würde.

314159265358979

@Code: LOL, den Typen würde ich entlassen. Drei-Sterne-Programmierer.

Ethon

314159265358979 schrieb:

@Code: LOL, den Typen würde ich entlassen. Drei-Sterne-Programmierer.

Tja, oft ist es aber besonders für die fähigsten Mathematiker/Physiker unter ihrem Niveau zu Programmieren. Und du würdest nicht die besten Leute raushauen nur weil sie übermäßig komplexe Programme raushauen, die machen was sie sollen. (Nicht a7uf das Beispiel bezogen)

314159265358979

Dann soll er nicht programmieren, sondern tun, was er kann.

Ethon

314159265358979 schrieb:

Dann soll er nicht programmieren, sondern tun, was er kann.

Dann musst du aber wiederrum einen Programmierer finden, der das Konzept versteht. Und da viele gute Programmierer sonst nix können (diese ganzen FH-Gestalten ) ist das dann auch höchstens geringfügig besser.

314159265358979

Ach, viel schlimmer gehts doch kaum.

curry-king

camper schrieb:

Hier lässt sich sogar ohne stilistische Änderungen einiges verbessern.
Wer löscht eigentlich die alten bool-Arrays? Ich vermisse da ein delete[].
Kann nat. auch an anderer Stelle des Programmes stehen... in diesem Fall wäre es allerdings diese geteilte Zuständigkeit, um die ich mich zuerst kümmern würde.

In meiner ersten überarbeiteten Version habe ich dieses Speicherleck schon geschlossen

curry-king

314159265358979 schrieb:

Dann soll er nicht programmieren, sondern tun, was er kann.

Der Urheber des Codes hat ne absolut saubere Masterarbeit in 6 Monaten hingelegt. Das der Code stilistisch schei sse und von Speicherlecks durchlöchert ist/war schmälert seine Arbeit nicht. Der Code ist ein proof-of-concept eines speziellen automata lernaers. Für seine Bedürfnisse völlig ausreichend. In seinem Fall hätte ich das Softwareengineering wohl auch ganz hinten angestellt...

hustbaer

@curry-king
Nimm Pi nicht zu ernst, der ist nur glücklich wenn er jmd. bashen kann.

314159265358979

So nen Code schreib ich nicht mal bei irgend nem komischen Konzept. Einfach grauenhaft.

Bashar

curry-king schrieb:

Der Code ist ein proof-of-concept eines speziellen automata lernaers.

Einen Proof-of-concept wirft man, wenn er funktioniert, weg und refaktorisiert ihn nicht.

hustbaer

Die Grenze zwischen refactoring und neu implementieren ist ziemlich schwammig. Ich sehe kein Problem darin, funktionierenden Proof-Of-Concept Code umzubauen.
Und SO katastrophal ist der Code nicht. Sieht für mich eher so aus, als ob der relativ einfach umzubauen wäre.

@314159265358979
Du bist aber auch das komplett andere Extrem wie der der den Code geschrieben hat.
Du bist C++ Sprachfetischist, dir kommt es mehr auf das "wie" an als darauf dass irgendwas fertig wird.
Der der den Code geschrieben hat hatte anscheinend kaum C++ Kenntnisse, und wollte das Teil einfach zum Laufen bekommen, alles andere war für ihn vermutlich total nebensächlich.

Deswegen ist der Code aber nicht total katastrophaler Müll, und muss jetzt geschmäht und weggeworfen werden.

phlox81

Würde den Code mal durch Cppcheck laufen lassen, dann hast du die gröbsten Schnitzer schon mal analysiert, und kannst evtl. memleaks korrigieren.