Adaptation Modern C++ sur du Legacy Code

Version imprimable

Bonjour !

Je dois maintenir une application construite avant le C++11. L’application fonctionne grosso modo de cette façon :

Voici le code d'avant :

Code:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
 
#include <iostream>
#include <list>
 
class A  // construite pour l'exemple ...
{
public:
	A(int _nbr, int _valeur){
		nbr=_nbr; valeur =_valeur;
	}
	~A(){};
 
	int getNbr() {return nbr;}  // pour avoir des fonctions à utiliser
	int getValue() {return valeur;}  // pour avoir des fonctions à utiliser
	void setValue(int _v) { valeur = _v;}  // pour avoir des fonctions à utiliser
private:
	int nbr;
	int valeur;
};
 
 
int main(int argc, char **argv)
{
	std::list<A*> myList;
	A* tmp = nullptr;
 
	//creation avec new
	for(int i=0;i<7; i++)
		myList.push_back(new A(i, 3*i-1));
 
	// affichage de la liste
	for(auto l : myList)
		std::cout << l->getNbr() << " " << l->getValue() << std::endl;
 
	//suppression des éléments impairs
	for(auto it = myList.begin(); it !=myList.end(); it++) {
		if ((*it)->getNbr()%2) {
			delete (*it);
			myList.erase(it);
			it--;
		}
	}
 
	// recherche d'un élément particulier i=4
	for(auto it = myList.begin(); it !=myList.end(); it++) {
		if ((*it)->getNbr()==4) {
			tmp = (*it);
			break;
		}
	}
	// plus tard ... modification de l'élément tmp
	if (tmp != nullptr) {
		tmp->setValue(-8);
	}
 
	// affichage de la liste
	for(auto l : myList)
		std::cout << l->getNbr() << " " << l->getValue() << std::endl;
 
	//suppression totale
	for(auto it = myList.begin(); it !=myList.end(); it++) {
			delete (*it);
			myList.erase(it);
			it--;
	}
	myList.clear();
 
	return 0;
}

J'ai travaillé sur le code et les smart-pointers afin de réduire la dette technique du logiciel :

Code:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
 
#include <iostream>
#include <list>
#include <memory>
#include <algorithm>
 
class A // construite pour l'exemple ...
{
public:
	A(int _nbr, int _valeur){
		nbr=_nbr; valeur =_valeur;
	}
	~A(){};
	int getNbr() {return nbr;}  // pour avoir des fonctions à utiliser
	int getValue() {return valeur;}  // pour avoir des fonctions à utiliser
	void setValue(int _v) { valeur = _v;}  // pour avoir des fonctions à utiliser
private:
	int nbr;
	int valeur;
};
 
 
int main(int argc, char **argv)
{
	std::list< std::shared_ptr<A> > myList;
	std::shared_ptr<A> tmp;
 
	//creation d'élements dans une liste
	for(int i=0;i<7; i++)
		myList.push_back( std::make_shared<A>(i, 3*i-1) );
 
	// affichage de la liste
	for(auto l : myList)
		std::cout << l->getNbr() << " " << l->getValue() << std::endl;
 
	//suppression des éléments impairs
	myList.remove_if( [](std::shared_ptr<A> a) { return a->getNbr()%2; } );
 
	//recherche d'un élément particulier i=4
	auto it = find_if(myList.begin(), myList.end(), [](std::shared_ptr<A> a) { return a->getNbr()==4; } );
	if (it != myList.end())
		tmp = (*it);
 
	//plus tard ... modification de l'élément tmp
	if (tmp != nullptr)
		tmp->setValue(-8);
 
	// affichage de la liste
	for(auto l : myList)
		std::cout << l->getNbr() << " " << l->getValue() << std::endl;
 
	//suppression totale
	myList.clear();
 
	return 0;
}

Je me suis débarrassé de la gestion des pointeurs et cela me ravit.

Toutefois, est ce que je peux encore améliorer mon travail en C++ "moderne" sur cet exemple ?

Je vous en remercie d'avance !

31/08/2018, 20h19
dalfab
Bonjour,
- remove_if() ne fait pas ce que tu crois, il ne supprime rien!
- utiliser des unique_ptr<> plutôt que des shared_ptr<> serait nettement plus optimum. Mais tu verras alors des lignes qui ne fonctionneront plus car elles n'étaient pas optimales. Par exemple l'utilisation de tmp serait à reposer.
- Il y des paramètres passés par valeur, il faut penser utiliser le passage par référence quand il est utile.
- la fonction clear() ne sert plus à rien car l'objet disparaît dès la ligne suivante
31/08/2018, 20h43
Bousk

- for(auto l : myList) fait une copie et donc une incrémentation/décrémentation du compteur de référence pour chaque objet
- Les shared_ptr ne remplacent pas des pointeurs nus, unique_ptr oui
- Ne pas manipuler d'objets dynamiques serait encore mieux
- vector est préférable à list
31/08/2018, 21h03
tickyletroll
Bonjour et merci pour votre réponse !

Pour le remove_if, je ne savais pas et je ne comprenais pas. Lorsque j'écrivais cela:
Code:

1 2 3 ~A() { std::cout << "destruction de " << nbr << std::endl;} myList.remove_if( [](std::shared_ptr<A> a) { return a->getNbr()%2; } );
La liste des objets supprimés s’affichait à l'écran après le remove_if.

Je viens de relire encore la doc et si j'ai compris, remove_if ne fait que de déplacer des objets mais qu'il ne les supprimait pas. Et si j'ai bien compris, les objets apparaissent comme supprimé car justement j'utilise des shared_ptr. (et sans cela, j'avais une belle fuite mémoire) C'est bien cela ?

Je devrais écrire plutôt quelque chose comme ceci ?
Code:

1 2 myList.erase(std::remove_if(myList.begin(), myList.end(), [](std::shared_ptr<A> a) { return a->getNbr()%2; } ), myList.end());
Je regarde et travaille tous les autres points que vous m'avez indiqué pour poster quelque chose de plus accadémique

Encore merci !!
31/08/2018, 21h47
jo_link_noir

std::list::remove_if supprime bien les éléments. C'est std::remove_if qui les déplace. du list.remove_if(...) ou list.erase(std::remove_if(...), end) font la même chose mais la première forme est plus efficace et simple.
31/08/2018, 21h55
dalfab

Oui, et oui.
remove_if() utilise des move() pour effectuer des transferts optimisés vers la fin, il peut laisser des pointeurs vides car après un remove() on s'engage à détruire les élément déplacés. Et je crains que si on supprime un élément déjà à la fin, même si c'est un shared_ptr<> le destructeur ne sera pas appelé. Il faut donc bien ajouter un erase() comme tu l'as fait.

J'essaie de prendre en compte vos remarques et je suis arrivé à ceci:

Code:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
 
#include <iostream>
#include <vector>
#include <memory>
#include <algorithm>
 
class A // construite pour l'exemple ...
{
public:
	A(int _nbr, int _valeur){
		nbr=_nbr; valeur =_valeur;
		std::cout << "Création de " << nbr << std::endl;
	}
	~A(){std::cout << "suppression de " << nbr << std::endl; };
	int getNbr() {return nbr;}  // pour avoir des fonctions à utiliser
	int getValue() {return valeur;}  // pour avoir des fonctions à utiliser
	void setValue(int _v) { valeur = _v;}  // pour avoir des fonctions à utiliser
private:
	int nbr;
	int valeur;
};
 
 
int main(int argc, char **argv)
{
	std::vector< std::unique_ptr<A> > myVector;
 
	//creation d'élements du vecteur
	for(int i=0;i<7; i++)
		myVector.push_back( std::make_unique<A>(i, 3*i-1) );
 
	//suppression des éléments impairs
	myVector.erase(std::remove_if(myVector.begin(), myVector.end(), [](std::unique_ptr<A> &a) { return a->getNbr()%2; } ), myVector.end());
 
	// affichage du vecteur
	for(auto const& l : myVector)
		std::cout << l->getNbr() << " " << l->getValue() << std::endl;
 
	//recherche d'un élément particulier i=4
	auto it = find_if(myVector.begin(), myVector.end(), [](std::unique_ptr<A> &a) { return a->getNbr()==4; } );
	if (it != myVector.end())
		(*it)->setValue(-8);
 
	// affichage du vecteur
	for(auto const& l : myVector)
		std::cout << l->getNbr() << " " << l->getValue() << std::endl;
 
	//suppression totale
	myVector.clear();
 
	return 0;
}

Est ce que cela semble plus propre ?