Discussion :

[Luc Hermitte]

Bonjour,

Je suis en train d'évaluer/analyser un outil d'analyse statique de code, dans sa dernière version, il y a une nouvelle vérification d'une règle MISRA C++ qui en substance exige que toute fonction virtuelle définie ne doive jamais être re-définie -- à quelques exceptions près pour les destructeurs et pour les fonctions virtuelles pures qui ont des implémentations par défaut.

Je gamberge depuis deux jours sur le sujet, et j'ai du mal à me faire une idée entre:
- ça n'apporte pas grand chose
- ça introduit des complications qui vont nous brider
- c'est un vrai gros plus.

Dans tous les cas, je n'ai pas trouvé d'exemples qui allaient vraiment dans un sens ou dans l'autre.

En gros, cela nous interdit les classiques

Code :

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

1
2
3
4
5
6
7
struct Child : Super {
    virtual void f() {
        foo()
        Super::f();
        bar();
    }
};

et ce n'est pas un mal. Bien au contraire -- oui, je préfère le DP Template Method.

Mais cela complique aussi les choses pour définir des comportements par défaut qui pourraient être spécialisés dans des classes filles. Ce qui interdirait ce genre de chose (que j'utilise dans mes cours)

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
24
25
26
struct DataRenderer : noncopyable{
    virtual ~DataRenderer() {}
    void render(DataList const& dataList_) {
        do_prologue();
        for (auto const& data : dataList_) { do_render(data); }
        do_epilogue();
   }
private:
   virtual void do_prologue() {} // oui il y a une implémentation par défaut qui n'en fout pas une
   virtual void do_epilogue() {} // oui il y a une implémentation par défaut qui n'en fout pas une
   virtual void render(Data const& data_) = 0;
}
 
struct XMLDataRenderer : DataRenderer {
private:
   virtual void do_prologue() override { cout << "<datalist>"; } // je viole la règle
   virtual void do_epilogue() override { cout << "</datalist>";} // je viole la règle
   virtual void render(Data const& data_) override { ... }
};
 
struct GnuplotDataRenderer : DataRenderer {
private:
   // ici, je profite de l'implémentation par défaut du prologue et de l'épiloque
   // et il en ira de même pour des sorties .csv/.tsv/...
   virtual void render(Data const& data_) override { ... }
};

Est-ce vraiment hérétique d'être pragmatique en profitant de ce comportement par défaut ?

[jblecanard]

Hello

Est-ce vraiment hérétique d'être pragmatique en profitant de ce comportement par défaut ?

Pour ma part, à ceci je répond non. Le comportement par défaut est particulièrement utile pour écrire des adapters.

[Médinoc]

Cela ressemble beaucoup à "Seules les classes feuilles de la hiérarchie peuvent être concrètes".

[Luc Hermitte]

@Medinoc, cela adresse d'autres aspects orthogonaux.
Le fait que ma classe de base est abstraite est un détail : il pourrait y avoir une autre classe dérivée qui spécialise un comportement, mais qui reste abstraite.

Après, il est vrai que ce que tu énonces interdit de redéfinir une fonction qui a déjà été définie.

[white_tentacle]

Je vais seulement faire une remarque, mais pour l’instant je me garderai bien de porter un jugement définitif.

Code c++ :

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

1
2
3
4
5
6
7
8
 void render(DataList const& dataList_) {
        do_prologue();
        // corps de fonctnio
        do_epilogue();
   }
private:
   virtual void do_prologue() {} // oui il y a une implémentation par défaut qui n'en fout pas une
   virtual void do_epilogue() {} // oui il y a une implémentation par défaut qui n'en fout pas une

et

Code c++ :

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

1
2
3
4
5
6
7
8
 
struct Child : Super {
    virtual void f() {
        foo()
        Super::f();
        bar();
    }
};

C’est la même chose, à la différence près que dans le premier cas, on a intégré dès le départ que le corps de la fonction pouvait être précédé d’un prologue / suivi d’un épilogue, alors que dans le deuxième on a fait ça un peu à la rache.

[KsassPeuk]

Lu'!

Autant si je trouve que le premier cas évoqué est effectivement intéressant, le gain n'est pas assez grand pour justifier qu'on sacrifie le fait d'avoir plusieurs niveaux de variations pour les hiérarchies de classe. Le principal c'est surtout que la fonction membre implémentée respecte le contrat défini par la classe parente (et inductivement si c'est un membre dont elle hérite elle-même).

Après, quand on utilise le NVI pour vérifier ce genre de contrat, le problème c'est qu'à la première implémentation, on ne peut plus vérifier les contrats de nouvelles variations. Si on veut le faire, ça passe par un nouveau niveau de NVI. Donc on se retrouve dans le cas évoqué ici : une première variation définie est finale.

Après, j'suis pas un pro moi hein.

[Nicolas Hamblenne]

Je me méfie des règles qui ne sont pas respectées par la lib standard -- elle n'est pas parfaite, mais quand on se demande si interdire est une bonne idée, le fait que ça interdirait une pratique de la lib standard est un signe que ce n'en est pas une. Et si la lib standard ne comporte pas beaucoup de classes conçues pour fonctionner avec des fonctions virtuelles aucune de celles que j'ai regardées (les exceptions, les facets, les IOStreams, toutes ont des fonctions virtuelles avec une implémentation qu'on peut remplacer dans les descendants) ne la respecte.

De même, Qt ne le fait pas (le traitements des événements est fait comme ça, et Qt est représentatif sur ce point).

De plus, dans un langage comme CLOS, c'est même une fonctionnalité du langage.

Ma conclusion, c'est peut-être bon dans le contexte de MISRA, mais dans un contexte plus général, la règle me semble être trop conservative.

[koala01]

Salut,

Je crois que, comme toutes les règles visant à interdire quelque chose, il y a toujours du bon et du moins bon car on pourra toujours trouver l'argument qui justifie cette interdiction et le contre argument qui plaidera en sa défaveur.

Je vais donc avoir une réponse de normand, mais une fois n'est pas coutume :
Oui, cette règle peut s'avérer utile dans bien des cas, mais elle risque de nous interdire certaines constructions qui pourraient s'avérer utiles dans des cas plus ou moins "border line". Je crois que plutôt que d'interdire la re-définition des fonction déjà définie, il serait donc plus correct d'inciter à ne pas redéfinir une fonction définie en demandant une justification précise de la raison pour laquelle nous voulons pouvoir la redéfinir.

Le problème, c'est que c'est typiquement le genre de finesse dont un outil d'analyse statique est parfaitement incapable

[ternel]

Je suis d'accord avec koala01:
En terme de règles de développement, "jamais" et "toujours" signifie en général "à moins que je puisse prouver sérieusement le contraire".
En outil d'analyse de code, "jamais" et "toujours" signifie "sans aucune exception".
C'est tout le problème d'un outil automatique: il n'est pas humain, et ne comprend pas la subtilité.

"Il faut toujours préférer une référence constante à une copie pour passer une classe en argument."
"Ne jamais utiliser un pointeur nu."
"Ne jamais écrire faire un cast."
"Ne jamais utiliser une implémentation par défaut d'une méthode virtuelle".

[Luc Hermitte]

@whitetentacle.
J'ai pris le premier exemple de DP Template Method qui me venait à l'esprit. Dans le cas usuel, il y a une énorme différence entre le DP-TM et l'appel explicite de la fonction parente : le premier cas de figure nous assure le respect des invariants. Dans le second cas, on fait aveuglement confiance à celui qui spécialise pour ne pas faire de bêtises. Et effectivement, dans le premier cas on a analysé le problème pour déduire les commonalities (je n'ai toujours pas trouvé de traduction pour ça), et les points de variations. C'est tout de même un cran au delà que l'approche à l'arrache, non ?

@Nicolas.
Malheureusement, beaucoup de règles (imposées par des standards ou proposées par des outils -- sous prétexte qu'il sont capables de les détecter) ne sont pas vérifiées dans des libs de qualité. Même des bonne règles comme le respect du SRP, chose que std::string ne vérifie absolument pas p.ex.

Cela me m'empêche pas de distinguer les cibles des règles. P.ex. pour un code métier/applicatif, je n'aurai aucune vergogne à interdire les pointeurs bruts. Pour des bibliothèques bas-niveau de type définitions de TAD/matrices, ..., je serai le premier à faire une exception.

@Koala.
Définitivement, les outils ne peuvent pas vérifier que l'on commente avec une bonne argumentation les cas où l'on ne respecte volontairement pas une règle. Au mieux, ils peuvent détecter un //tag ou [[autre]] #pragma que l'on laisse trainer, mais pour l'argumentation, ils ne pourront rien faire.


----
Sinon, je pars pour ne pas intégrer cette vérification automatisée dans notre référentiel car trop contraignant, et de plus cela peut conduire à de l'over-engineering. Imaginez, que pour raison d'ISP & cie, on sépare le prologue vide de l'épilogue vide. Ainsi, pour du YAML, on n'a pas d'épilogue, mais pour un format avec tag de fin à la EOF, on n'a pas de prologue. Résultat, on se retrouverait vite avec de l'héritage multiple et des diamants si on veut proposer des implémentations par défaut. Clairement, on n'y gagnerait pas.

[bacelar]

Le problème, c'est que c'est typiquement le genre de finesse dont un outil d'analyse statique est parfaitement incapable

Les outils d'analyses statiques .NET comme FxCop ou code Analysis permettent de désactiver une règle à un endroit précis du code via l'utilisation d'attributs qui permettent de spécifier un argumentaire pour contrevenir à la règle.
L'outil ne fait que vérifier la présence de l'attribut, mais cela permet déjà de se poser les bonnes questions et d'éviter les erreurs d'étourderie.

Y a pas d'équivalent pour les outils C++ ?

[Luc Hermitte]

Chaque outil est unique et utilisera une façon ou une autre pour dire: "oublie la règle ici". Voire, il ne le permettra pas.
Bref, il n'y a pas de convention commune entre clang-analyse, OCLint, clang-tidy, Coverity, CppDepend, Understand for C++, Klockworks, VS, et c'est tout juste si j'en ai listé la moitié qui existe.

[bacelar]

Pas de comité de normalisation pour ce type de chantier ?

[jblecanard]

Pas de comité de normalisation pour ce type de chantier ?

Les attributs généralisés peuvent faire le job, mais d'ici à ce que les softs les adoptent, il y a de l'eau qui va couler sous les ponts.

[white_tentacle]

@whitetentacle.
J'ai pris le premier exemple de DP Template Method qui me venait à l'esprit. Dans le cas usuel, il y a une énorme différence entre le DP-TM et l'appel explicite de la fonction parente : le premier cas de figure nous assure le respect des invariants. Dans le second cas, on fait aveuglement confiance à celui qui spécialise pour ne pas faire de bêtises. Et effectivement, dans le premier cas on a analysé le problème pour déduire les commonalities (je n'ai toujours pas trouvé de traduction pour ça), et les points de variations. C'est tout de même un cran au delà que l'approche à l'arrache, non ?

En fait, de mon point de vue le premier cas va arriver lorsque tu fais ta conception en amont, le second lorsque que tu modifies l’existant et que tu ne peux pas te permettre de refactoring.

Sinon, j’ai regardé un peu ma base de code, afin de voir dans quelle mesure je respectais cette règle intuitivement. Globalement, quelques remarques :
- j’ai très peu d’héritage à plus d’un niveau. Ça limite énormément les cas.
- il y a des endroits ou j’ai déclaré volontairement virtuelles des fonctions définies --> je me suis laissé volontairement la possibilité de violer la règle pour plus tard au besoin. Le problème est que je ne connais pas forcément mes besoins futurs, ni mes points de variation exacts, et que je veux pouvoir autant que possible garder la compatibilité binaire --> j’aurais pu faire une interface virtuelle, mais on revient au problème de l’overengineering.
- un double dispatch sur une hiérarchie de classes concrètes. Là je ne sais même pas comment faire autrement en fait (après on peut discuter aussi de l’intérêt d’une hiérarchie de classes concrètes, cf ce que disait Médinoc…).

Je crois effectivement que la règle n’est pas mal en soi, mais trop restrictive hors de certains contextes particuliers type misra où on a l’habitude de vivre avec de telles restriction. Cela dit, je pense que c’est une bonne métrique : s’il y a trop de violations de cette règle, il y a probablement un soucis de conception dans le code.

Sinon, je pars pour ne pas intégrer cette vérification automatisée dans notre référentiel car trop contraignant, et de plus cela peut conduire à de l'over-engineering. Imaginez, que pour raison d'ISP & cie, on sépare le prologue vide de l'épilogue vide. Ainsi, pour du YAML, on n'a pas d'épilogue, mais pour un format avec tag de fin à la EOF, on n'a pas de prologue. Résultat, on se retrouverait vite avec de l'héritage multiple et des diamants si on veut proposer des implémentations par défaut. Clairement, on n'y gagnerait pas.

En fait, pour ce genre de cas, je pense que des politiques, template ou en tant que membre, peuvent être plus adaptées que de l’héritage.