PrepaC Retours FR 2015

De Ensiwiki
Aller à : navigation, rechercher

Retours 2014/2015

fond

  • Pour les traitements binaires, utilisez les opérateurs bit-à-bit [1].
  • Il existe de nombreuses astuces pour manipuler les bits. En voici une inspirée d'un rendu brillant
uint16_t ensemble_cardinal(ensemble_t e) {
        uint16_t res=0;
        for (; e; e &= e - 1)
                res++;
        return res; 
}                                                         

Le livre "Hacker's Delight" est rempli d'astuce en ce genre.

  • Après un malloc, vérifier que l'allocation s'est bien passée.
  • Quand on alloue (avec malloc/calloc ou avec un constructeur spécifique comme creer_liste_suspects), on libère (avec free ou avec un destructeur spécifique comme detruire_liste_suspects).
  • Le mot clé const dans le prototype d'une fonction signifie qu'on n'a pas le droit de modifier le contenu du paramètre. Ainsi la fonction
 struct suspect *creer_suspect(const char *nom, ensemble_t attributs) 

n'autorise pas la modification des caractères de la chaîne nom. En conséquence, il est dangereux (d'où les warnings donnés par GCC) d'affecter nom dans une chaine non constante (genre le champ nom de struct suspect), qui pourra réutiliser ailleurs sans respecter la consigne const. En bref, il fallait allouer une nouvelle chaîne pour répondre correctement au problème.

Et si je suis très malin et que j'enlève le const du char *nom, je peux me dispenser d'allouer la chaine suspect->nom ? Et bien non, même dans le cas de la fonction

 struct suspect *creer_suspect(char *nom, ensemble_t attributs) 

il est aussi recommandé d'allouer suspect->nom de la façon suivante

 struct suspect *nouveau_suspect = malloc(sizeof(struct suspect));
 assert(nouveau_suspect != NULL);
 nouveau_suspect->nom = malloc(sizeof(char) * (strlen(nom)+1));
 assert(nouveau_suspect->nom != NULL);
 strcpy(nouveau_suspect->nom, nom);

Pourquoi cela ? Parce qu'une simple affectation nouveau_suspect->nom = nom, ferait dépendre nouveau_suspect->nom d'une allocation de mémoire extérieure au module suspect. Rien n'empêche qu'au retour de la fonction creer_suspect, le paramètre nom soit désalloué immédiatement après rendant ainsi nouveau_suspect->nom très instable car ne pointant plus sur une zone mémoire allouée pour le programme.

  • Le code rendu ne doit pas comporter de warning (-Wall -Wextra), ne comporter ni fuites mémoires ni accès illégaux (valgrind).
  • valgrind --leak-check=full <nom_programme> permet d'avoir une localisation très précise des fuites mémoire et accès illégaux.
  • Lorsqu'on utilise strlen, la valeur rendue ne tient pas compte du caractère de fin de chaîne. Attention, en cas de recopie aux accès illégaux.
  • Lorsqu'on manipule une chaîne comme un tableau de char, ne pas oublier d'affecter le caractère \0 à la fin (et avoir assez de place pour ce caractère en plus). Exemple pour stocker la chaine "ta":
        char tata[3]; // tata[2] faux: pas la place pour le \0 final
        tata[0] = 't';
        tata[1] = 'a';
        tata[2] = '\0'; // si on oublie, tata n'est pas une chaine correcte et les conséquences peuvent etre graves
  • Pour retirer un élément d'une liste doublement chainée, on n'a pas besoin de parcourir cette liste pour trouver la position de l'élément à supprimer. Il suffit de chainer l'élément précédent avec l'élément suivant et de libérer la mémoire occupée par l'élément supprimé. Les éléments suivant et précédent sont directement accessibles depuis l'élément à supprimer, puisqu'on manipule une liste double chainée (pointeur vers le précédent et le suivant inclus dans la structure de données représentant une cellule).
  • Le code suivant est méritant, mais dangereux:
        truc *toto = malloc(N * sizeof(truc));
        if (toto == NULL) {
                fprintf(stderr, "Erreur d'allocation!\n");
        }

Il est méritant parce que son auteur a pensé à vérifier la valeur de retour de malloc, mais il est dangereux parce qu'on autorise le programme à continuer quand même. Dans ce cas, le mieux est d'utiliser la fonction perror() qui affiche un message correspondant au type d'erreur positionné par malloc(), puis d'utiliser exit() pour terminer le programme. La chaine de caractère passée en paramètre de perror sera affichée avant le message d'erreur.

        truc *toto = malloc(N * sizeof(truc));
        if (toto == NULL) {
                perror("dans creer_suspect:");
                exit(1);
        }
  • Le gros intérêt d'utiliser des champs de bits est de pouvoir construire des masques pour simplifier votre code et le rendre plus efficace.

Par exemple, il est dommage d'ajouter les éléments d'une caractéristique un à un lorsqu'on a obtenu une réponse:

         if (num_quest >= 5 && num_quest < 8) {
                 /* type de coiffure */
                 ensemble_ajouter_elt(question, COIFFURE_CHAUVE);
                 ensemble_ajouter_elt(question, COIFFURE_COURT);
                 ensemble_ajouter_elt(question, COIFFURE_LONG);
         }

alors qu'on aurait pu simplement définir un ensemble représentant toutes les caractéristiques du même type (par exemple le type de coiffure), et mettre à jour l'ensemble des questions posées à l'aide d'une simple union:

        #define TYPE_COIFFURE (COIFFURE_CHAUVE | COIFFURE_COURT | COIFFURE_LONG)

         if (num_quest >= 5 && num_quest < 8) {
                 question = ensemble_union(question, TYPE_COIFFURE);
         }
  • Le code suivant est dangereux:
        char reponse[2];
        printf("Répondre par 'o' ou 'n':\n");
        scanf("%s", reponse);

Un utilisateur mal intentionné (comme n'importe quel correcteur de fil rouge) aura vite fait de faire planter votre programme en répondant "pouet" à cette question, puisque scanf essaiera de stocker la chaine de caractère "pouet" dans la zone mémoire reponse occupant l'espace de deux caractères. Le code est encore pire si on met un char reponse[1]; il n'y aura même pas la place pour stocker le '\0' de fin de chaine après une réponse 'o' ou 'n', erreurs d'accès mémoire et algorithmiques quasiment assurées dans ce cas.

  • Ah les sizeof()...
Soit name, une chaine déclarée en char* et contenant "Sebastien" (9 caractères "visibles" + 1 caractère "invisible" \0). 
Voilà ce que donne sous un centos Ensimag les résultats de différents sizeof() :
        sizeof(name) = 8
        sizeof(char*) = 8
                ==> sizeof(name) renvoie donc la taille d'un pointeur
                    sur un caractère et non la place nécessaire en mémoire
                    pour stocker tous les caractères de la chaine "Sébastien"
        sizeof(*name) = 1
        sizeof(char) = 1
                ==> sizeof(char) renvoie la taille nécessaire pour stocker un seul caractère en mémoire
                ==> sizeof(*name) renvoie aussi 1, la taille nécessaire pour stocker le premier caractère name[0]
        strlen(name) = 9
                ==> strlen ne compte pas le \0 de fin de chaine
        sizeof(char) * (strlen(name)+1) = 10
                ==> voilà la seule bonne formule pour connaitre la taille nécessaire
                    pour stocker en mémoire la chaine name = "Sébastien"

Recommandation : utilisez sizeof avec des types de données plutôt que des variables, cela évite les erreurs.

  • Nesting : quésaco ?

Le nesting consiste à déclarer / encapsuler une fonction à l'intérieur d'une autre. Par exemple :

void creer_liste_suspects(struct liste_suspects* l)
{
        // Fonction creer_suspect à l'intérieur de creer_liste_suspects
        struct suspect * creer_suspect(const char *name)
        {
                // ...
        }

        // On utilise creer_suspect
        struct suspect* new_suspect;
        new_suspect = creer_suspect("Sébastien");
        // ...
}

Est ce que c'est possible en C : techniquement oui mais seulement parfois selon l'architecture et le gcc, ça l'est sur l'environnement centos Ensimag. Plusieurs niveaux d'imbrication sont même possibles.

Est ce que c'est conseillé : NON car ce n'est pas standard du tout en plus d'être illisible et sujet à erreurs.

Conclusion : le nesting est à bannir en C

  • Une erreur bête

Le code suivant

ensemble_t questions = ensemble_vide;

semble déclarer un ensemble_t questions et l'initialiser à ensemble_vide en appelant cette dernière fonction.

Sauf qu'il manque les parenthèses lors de l'appel de la fonction. Le code correct est le suivant :

ensemble_t questions = ensemble_vide();

Sans les parenthèses, ensemble_vide est l'adresse de la fonction ensemble_vide en mémoire. Cette adresse est interprétée comme un type ensemble_t et donne donc des résultats totalement imprévus mais en tout cas pas un ensemble vide. Le compilateur vous met d'ailleurs sur la voie par le message

src/jeu.c: In function ‘main’:
src/jeu.c:67: attention : initialization makes integer from pointer without a cast

forme

  • Evitez de déclarer des variables qui ne servent à rien. Par exemple:
        ensemble_t vide = 0;
        return vide;

est équivalent à:

        return 0;
  • Le code suivant n'accomplit probablement pas ce à quoi vous pensiez:
	if (n == 0)
		printf("Est-ce que la personne coupable est un homme?\n");
		questions_posees = questions_posees + HOMME + FEMME;
	if (n == 1)
		printf("Est-ce que la personne coupable est une femme?\n");
		questions_posees = questions_posees + HOMME + FEMME;
	if (n == 2)
		printf("A-t-il une moustache?\n");
		questions_posees = questions_posees + MOUSTACHE;
         ...

L'indentation du code ci-dessus est trompeuse, la condition ne portant ici que sur la ligne qui suit l'instruction if (...). En effet, la syntaxe standard d'un test en C est de cette forme:

        if (condition) {
                // action si condition == true
        }

Dans le cas particulier où l'action se résume à une instruction ou un appel de fonction, on peut se permettre d'oublier les accolades englobantes (même si c'est fortement déconseillé lorsqu'on apprend à programmer en C). En revanche, si l'action à effectuer est un bloc de code, la syntaxe associée à la création d'un bloc C doit être utilisée (c'est-à-dire { ... } ).

Le code corrigé ressemble à ça, mais n'est pas encore pleinement satisfaisant:

	if (n == 0) {
		printf("Est-ce que la personne coupable est un homme?\n");
		questions_posees = questions_posees + HOMME + FEMME;
	} else if (n == 1) {
		printf("Est-ce que la personne coupable est une femme?\n");
		questions_posees = questions_posees + HOMME + FEMME;
	} else if (n == 2){
		printf("A-t-il une moustache?\n");
		questions_posees = questions_posees + MOUSTACHE;
        }
         ...

En effet, on se trouve dans une situation où on effectue des actions différentes en fonction de la valeur d'une variable entière qu'on énumère: on préfèrera utiliser la construction switch() dans ce cas.

        switch(n) {
	case 0:
		printf("Est-ce que la personne coupable est un homme?\n");
		questions_posees = questions_posees + HOMME + FEMME;
                break;
	case 1:
		printf("Est-ce que la personne coupable est une femme?\n");
		questions_posees = questions_posees + HOMME + FEMME;
                break;
	case 2:
		printf("A-t-il une moustache?\n");
		questions_posees = questions_posees + MOUSTACHE;
                break;
        default:
                printf("Mauvaise valeur de n!\n");
         ...
        }
  • Cherchez à écrire votre code de manière compacte. Par exemple,
        return a;

est préférable à

        if (a == true) {
                return true;
        } else {
                return false;
        }
  • Constantes binaires

Beaucoup d'entre vous ont déclaré un ensemble plein de cette façon:

        ensemble_t plein = 0b1111111111111111;

Remarquez que ces lignes déclarent le même ensemble, et ce quelle que soit la taille du type ensemble_t:

        ensemble_t plein = ~0;
        ensemble_t toujours_plein = -1; // attention, ensemble_t doit représenter un entier non signé

Elles présentent de plus l'avantage de marcher quelle que soit la taille du type ensemble_t... voir remarque suivante.

  • Evitez les "nombres magiques" dans vos codes [2]. Par exemple pour parcourir les bits d'un ensemble_t e:
        for (int i=0; i< sizeof(ensemble_t)*CHAR_BIT; i++) {...}
        // bien mieux que:
        for (int i=0; i< 16; i++) {...}

Ainsi si on veut changer la taille du type ensemble_t on peut simplement le redéfinir:

typedef uint64_t ensemble_t;


  • Préférez les opérations simples (|,&,^,~,<<,>>) aux opérations complexes (*,/,%, les fonctions flottantes de math.h). Votre code sera plus rapide. Un exemple :
        tmp = pow(2, numero_elt);

peut s'écrire à l'aide d'un décalage:

        tmp =1 << numero_elt; // la valeur '1' est décalée de numero_elt bits à gauche, tmp vaut donc bien 2 à la puissance numero_elt
  • opérateur complémentaire : ~ et non -x-1
  • Plus généralement, on utilise les opérateurs bit-à-bit pour faire les opérations sur des champs de bits plutôt que des opérateurs arithmétiques.
  • masquage avec &, union avec |
  • Le code rendu doit être bien commenté et respecter un style de codage homogène
  • /**/ et // servent uniquement pour les commentaires. Pour commenter du code, utiliser #if 0 ... #endif
  • Attention à la lisibilité de votre code. Le code suivant est peu lisible:
	(*l).nb_suspects=(*l).nb_suspects+1;
	(*(*l).queue).suiv = s;
	(*s).prec = (*l).queue;
	(*l).queue=s;

Le code suivant est un peu plus lisible pour un programmeur C expérimenté: il utilise l'opérateur -> pour accéder aux champs d'une structure depuis un pointeur vers cette structure, et utilise l'opérateur d'incrémentation qui rend le code plus concis:

	l->nb_suspects++;
	l->queue->suiv = s;
	s->prec = l->queue;
	l->queue = s;
  • static, c'est pour faire joli ou c'est vraiment utile ?

A lire une grande partie de vos rendus de fil rouge qui ne comportent aucune déclaration static, ce doit être pour faire joli.

En réalité, c'est vraiment utile et il est fortement recommandé de déclarer en static les fonctions qui ne servent qu'à l'intérieur d'un module module_a.c et n'ont pas à être appelées depuis un autre module module_b.c

La fonction suivante

static void fonction_exemple(void)
{
        // ...
}

verra son étiquette disparaitre à l'édition de liens, ce qui évitera de facto les liens externes. C'est donc une façon de sécuriser encore mieux cette fonction et accessoirement de ne pas avoir de problèmes de noms de fonctions potentiellement redondants entre des modules différents voire avec des librairies.

Si vous utilisiez des variables globales à l'intérieur d'un module module_a.c, il serait aussi recommandé de les déclarer en static afin de mieux les protéger vis à vis des modules extérieurs. Mais comme les variables globales, c'est pas bien sauf en de très rares occasions exceptionnelles et motivées, vous ne devriez jamais avoir ce problème...

static est aussi une solution pour déclarer une variable globale mais locale à une seule fonction. Ainsi, dans la fonction suivante

void fonction_a(void)
{
        static int compteur_entrees_fonction_a = 0;
        compteur_entrees_fonction_a ++;
        // ...
}

La variable compteur_entrees_fonction_a est locale à la fonction fonction_a mais a une durée de vie globale. C'est à dire qu'elle occupera la même zone mémoire durant toute la vie du programme. L'initialisation "= 0" ne sera appliquée qu'au premier appel de fonction_a. Concrètement, compteur_entrees_fonction_a permettra de compter le nombre d'appels à fonction_a à l'intérieur de cette même fonction.

Rendus

  • Donner la bonne extension à vos rendus (.tar différent de .tgz)
  • Rendez de préférence vos projets dans des archives compressés. Le format .tgz répond très bien au besoin des projets.
  • Mettre un README dans chaque rendu pour décrire rapidement vos projets (ce qui marche ou pas, les extensions, un guide utilisateur)
  • Nettoyer votre archive en enlevant les fichiers inutiles : pas de ~fichier #fichier .swp .git, .o, exécutable, répertoires inutiles ...
  • Laissez bien dans votre rendu tous les répertoires et fichiers fournis (par exemple le répertoire testsuite...)