Discussion :

[cyrill.gremaud]

bonjour,

J'ai un soucis lorsque je dois ajouter un element à une liste un peu complexe. Voici mes structures :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
struct reader_send_list{
	struct shm_data *data;				/**< pointer to the first shm_data of the current list */
	struct reader_send_list *next;		/**< pointer to the next list */
};
 
struct reader_printhead{
	uint8_t ph_id;
	struct reader_send_list *next_frame;			/**< pointer to the next frame to send list */
	struct reader_printhead *next_printhead;		/**< pointer to the next printhead */
};

La structure printhead permet de chainer des éléments printhead comme elle mais contient un pointeur vers une structure reader_send_list. La structure reader_send_list est aussi une liste et je n'arrive pas à ajouter une élément à cette chaine au travers du pointeur vers la structure printhead.

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
 
extern void reader_append_send_list(struct reader_printhead *p_printhead)
{
	struct reader_send_list *p_new_send_list = malloc(sizeof *p_new_send_list);
 
	if((p_printhead->next_frame != NULL) && (p_new_send_list != NULL))
	{
		p_new_send_list->data = NULL;
		p_new_send_list->next = NULL;
 
		struct reader_send_list *current_list = p_printhead->next_frame;
		while(current_list != NULL)
		{
			printf("current_list=%p\n",current_list);
			current_list = current_list->next;
		}
		printf(" NULL = current_list=%p\n",current_list);
		current_list = p_new_send_list;
		printf("current_list=%p\n",current_list);
	}
}

et lorsque j'appelle ma fonction :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
 
printf("t_ph[0]->next_frame->next = %p\n",t_ph[0]->next_frame->next);
	reader_append_send_list(t_ph[0]);
	printf("t_ph[0]->next_frame->next = %p\n",t_ph[0]->next_frame->next);

Je précise que lors de l'initialisation de la structure printhead, le pointeur next_frame ne pointe pas vers null mais est attaché a une struct reader_send_list. mais je ne comprend pas mon erreur.

merci de votre aide

[diogene]

Le chainage n'est pas réalisé : Lorsqu'on quitte la boucle while(current_list != NULL), current_list est NULL et on a perdu le dernier maillon de la liste. L'instruction current_list = p_new_send_list; ne fait que modifier la variable locale et ne réalise pas le chainage.

La boucle devrait être while(current_list->next != NULL) et l'affectation current_list->next = p_new_send_list;.

Autres remarques :
- la fonction suppose que p_printhead != NULL. C'est peut être implicite dans le reste du code
- la fonction suppose p_printhead->next_frame != NULL, donc que la liste en question n'est pas vide. C'est une restriction un peu abusive à mon sens. La fonction devrait pouvoir traiter le cas où on ajoute un élément à une liste vide.
- comme tu ajoutes à la fin de la liste, regarde si tu n'as pas intérêt à ajouter au descripteur de liste un pointeur sur le dernier élément de la liste.

[ternel]

Bonjour,
Tout d'abord, quelques points de correction:

reader_append_send_list est fausse:
si le code n'entre pas dans le if, la mémoire a été allouée et ne sera jamais rendue.
une fonction "extern" dont le code est donnée? quelle horreur, ou juste une faute de copié collé pour le forum
Pourquoi une fonction pour ajouter un élément vide sans avoir en retour un pointeur sur l'élément ajouté?

Ton erreur vient du fait que current_list n'est pas un pointeur sur le pointeur contenu dans la struct, mais une copie locale du dit pointeur.

Essaie plutot cette fonction, qui ajoute un élément indiqué au bout d'une liste.

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
void append_reader_send_list(struct reader_send_list* *target_list, struct reader_send_list* appended)
{
	/* do nothing with empty arguments */
	if (target_list==NULL || appended==NULL) return;
 
	if (*target_list==NULL) {
		*target_list = appended;
	} else {
		struct reader_send_list* duplicate = *target_list;
		//set a duplicate to point to the first "next" having a null "next"
		while (duplicate->next != NULL) {
			duplicate = duplicate->next;
		}
		duplicate->next = appended
	}
}

Le code d'appel serait alors:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
	printf("t_ph[0]->next_frame->next = %p\n",t_ph[0]->next_frame->next);
	struct reader_send_list *p_new_send_list = malloc(sizeof (struct reader_send_list) );
	if(p_new_send_list != NULL)
	{
		p_new_send_list->data = NULL;
		p_new_send_list->next = NULL;
		append_reader_send_list( &(t_ph[0]->next_frame), p_new_send_list );
		printf("t_ph[0]->next_frame->next = %p\n",t_ph[0]->next_frame->next);
	}

Cela présente plusieurs avantages:

• la fonction est aussi valable pour ajouter dans une autre structure que reader_printhead.
• la fonction permet d'ajouter n'importe quel élément fourni (sauf null), donc permet d'initialiser avant.
• la fonction ne faisant pas d'allocation elle-même, la responsabilité de la gestion de la mémoire est correctement donnée à un seul scope, l'appelant.
• la fonction est directement réutilisable pour d'autre structures chainées (il suffit de la recopier en changeant les types.

[cyrill.gremaud]

Le chainage n'est pas réalisé : Lorsqu'on quitte la boucle while(current_list != NULL), current_list est NULL et on a perdu le dernier maillon de la liste. L'instruction current_list = p_new_send_list; ne fait que modifier la variable locale et ne réalise pas le chainage.

La boucle devrait être while(current_list->next != NULL) et l'affectation current_list->next = p_new_send_list;.

Autres remarques :
- la fonction suppose que p_printhead != NULL. C'est peut être implicite dans le reste du code
- la fonction suppose p_printhead->next_frame != NULL, donc que la liste en question n'est pas vide. C'est une restriction un peu abusive à mon sens. La fonction devrait pouvoir traiter le cas où on ajoute un élément à une liste vide.
- comme tu ajoutes à la fin de la liste, regarde si tu n'as pas intérêt à ajouter au descripteur de liste un pointeur sur le dernier élément de la liste.

Merci pour ta réponse. Mon code est maintenant fonctionnel. Je vais étudier encore les remarques de l'autre réponse et modifier mon code.

[cyrill.gremaud]

Donc voici mon code actuel pour cette fonction en prenant en compte certaines de vos remarques.

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
struct reader_send_list* reader_append_send_list(struct reader_send_list *p_send_list)
{
	struct reader_send_list *p_new_send_list = malloc(sizeof *p_new_send_list);
 
	if((p_send_list != NULL) && (p_new_send_list != NULL))
	{
		p_new_send_list->data = NULL;
		p_new_send_list->next = NULL;
 
		struct reader_send_list *p_current_list = p_send_list;
		while(p_current_list->next != NULL)
		{
			p_current_list = p_current_list->next;
		}
		p_current_list->next = p_new_send_list;
	}
	else
	{
		free(p_new_send_list);
		return(NULL);
	}
	return(p_new_send_list);
}

C'est déjà mieux non ? Je me demandais si je voulais pas intégrer le test de pouvoir ajouter un élément si jamais printhead->next est a null et dans l'initialisation du printhead, je met a null le next...

[ternel]

dans l'absolu, je séparerai les deux parties du if.

la première est un controle d'argument, la fonction devrait retourner immédiatement, sans faire une paire malloc-free.

La seconde est une protection contre l'erreur d'allocation, retourner en corriger les changements précédents (ici rien) est en général une bonne idée.

Plus ces contrôles sont traités tôt, moins le code est imbriqué (et indenté), et donc plus lisible. En effet, il n'y a pas besoin de else après un if(something) return ???;

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
struct reader_send_list* reader_append_send_list(struct reader_send_list *p_send_list)
{
	if (p_send_list == NULL) return NULL;
	/* *p_send_list is a list */
	struct reader_send_list *p_new_send_list = malloc(sizeof *p_new_send_list);
 
	if (p_new_send_list == NULL) return NULL;
 
	/* allocation is successful */
	p_new_send_list->data = NULL;
	p_new_send_list->next = NULL;
 
	struct reader_send_list *p_current_list = p_send_list;
	while(p_current_list->next != NULL)
	{
		p_current_list = p_current_list->next;
	}
	p_current_list->next = p_new_send_list;
	return(p_new_send_list);
}

[jouana]

Et pourquoi ne pas découper un peu plus ton code histoire d'avoir un truc un peu plus réutilisable à l'avenir.

du style

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
 
 
struct s_list
{
   void* item
   struct s_list* next;
}
 
struct reader_printhead{
	uint8_t ph_id;
	struct s_list* list_frame;			/**< pointer to the next frame to send list */
};

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
 
 
void    push_in_list(struct s_list** lst, void* item)
{
     if (lst == NULL || *lst == NULL)
         return;
 
     s_list tmp = *lst;
     if ((s_list* newElem = malloc(sizeof(s_list))) == NULL)
         return;
 
       newElem->item = item;
       newElem->next = NULL;
 
       while(tmp->next != NULL)
           tmp = tmp->next;
 
 
      tmp->next = newElem; 
}

ça te permet d'avoir un code un réutilisable et à ne plus te poser de questions quand tu manipule des listes chainées

Je te laisse voir pour la gestion d'erreurs.

Ta première stucture devient inutile tu peux tu peux directement mettre la struct contenu dans cette dernière dans la liste

[ternel]

Tu rejoins ma première proposition.
Mais je ne suis pas sûr que d'utiliser un void* soit toujours un bon choix, puisque tu y perds le typage.

[jouana]

Oui c'est sur après rien ne t’empêche de mettre un cast au besoin, ça permet surtout de généraliser le code et d'éviter de refaire x fois la même chose

[cyrill.gremaud]

merci à tous pour vos réponses