ProjetM1MPRI2013 / central

Repo principal
6 stars 0 forks source link

Returning reference to local variable (scenario + ... + network) #20

Closed dbaelde closed 10 years ago

dbaelde commented 10 years ago

Bonjour,

En compilant le code avec -Werror (ce que je recommande de nouveau, et que nous allons bientôt imposer de toute façon) je tombe sur le problème mentionné dans le titre de l'issue. De mémoire il s'agit de cette fonction:

std::string & Action::toString(){
  std::string s = "TODO";
  return s;
}

Pas besoin de valgrind pour voir qu'il y a un problème ici. Le même problème est présent à de nombreux autres endroits, en fait c'est la classe AbstractMessage du réseau qui demande ce type pour toString, ce qui pousse à l'erreur. Pourquoi ne pas avoir choisi std::string AbstractMessage::toString()?

mheinric commented 10 years ago

Effectivement, bonne question. A l'origine c'était pour éviter d'avoir à recopier la chaîne de caractère qui est renvoyée. Mais avec cette perspective il aurait sans doute mieux valu que je mette std::string* en type de retour, ce qui aurait sans doute créé moins de confusion. Je vais m'occuper de changer ça.

dbaelde commented 10 years ago

Avec un std::string dans le code mentionné plus haut, il n'y aurait pas de copie non plus (en coulisse, le compilo fait ce que tu avais probablement en tête avec une référence, mais la gestion mémoire devient correcte car il ne détruira pas l'objet renvoyé). Certes une nouvelle chaîne est créée chaque fois, mais ce n'est pas la fin du monde, c'est déja le cas dans le code actuel, et pour le coup cela te garantit que tu peux faire ce que tu veux avec la chaîne renvoyée par toString: la garder un moment sans crainte que quelqun te la modifie ou détruise, la modifier si cela te plait, etc.

mheinric commented 10 years ago

Je suis d'accord, c'était pas une bonne idée. C'est modifié

dbaelde commented 10 years ago

Super, merci!