[Dcmlib] GDCM commit

[Eric Boix]

	Salut Mathieu,

NOTE POUR JPR: Jean-Pierre, je pense que l'email de Mathieu s'addresse
  essentielement a toi. Je pense avoir deja fait les critiques que Mathieu
  fait maintenant...sans changement radical d'attitude de ta part.
  Fabrice Bellet se faisait aussi l'echo de ces critiques mais il a jete'
  l'eponge depuis longtemps. Sincerement, si DCMTK avait ete plus ouvert
  (une mailing list, un acces cvs et des package plus frequents), il y
  longtemps que j'aurais moi aussi laisse' tomber...
  A ta decharge, tu pratiques le C++ depuis peu de temps, et les DICOMries
  ne sont pas un sujet facile, avec tant d'exceptions a gerer. 
  Nous avons la chance que Mathieu accepte de nous aider, et je crois que
  c'est ta derniere chance pour que gdcm survive. Aussi je te demande
  humblement et a genoux, de te ne pas t'aigrir, d'essayer de repondre
  sur le fond et non en t'arc-bouttant sur des exemples et de changer
  profondement tes habitudes au lieu de persister dans la fuite avant
  dans la course au feature, pour eviter de nettoyer ton code...
  Aussi, detens toi, incline ton siege en arriere, respire profondement
  et dis toi qu'il ne faut jamais en vouloir a celui qui te signale
  que ton nez est sale, mais plutot le remercier de t'aider a etre plus
  propre...
  Je pense qu'il serait dommage d'echouer si pres du but et que si notre
  laboratoire pouvait enfin lire des images DICOM, cela permettrait
  au permanents ou a des gens comme moi, de faire un peu de science
  ou au moins des traitements d'image avanc'es, au lieu de perdre notre temps
  a patcher du code (volontairement?) abscons.
  Voila, je crois avoir suffisament contribue' a gdcm pour prouver que
  ma demande est sincere...et humble.
  Courage, tu peux le faire !


Une remarque preliminaire: JPR a profondement change' le code interne
(pour pouvoir traiter correctement les sequences, ce qui n'est pas simple)
ce qui explique la grande instabilite' du code ces jours derniers.
Les choses se clarifient, mais je pense qu'il est urgent (pour accelerer
les choses) de mettre dans la test suite, sous ctest, les choses que Mathieu
utilise ET qui doivent marcher. Cela eviterait de casser ce qui 
marchait avant, ce qui est le B, A, BA...

Le pb crucial est maintenant le Write qui n'est pas dans ctest.
En python, je faisais la chose suivante:
  * TestWrite gdcmData/source[i].dcm dest[i].raw
  * md5 dest[i].raw > md5.sig
  * comparaison de md5.sig avec une signature calcule'e precedement
    et valide'e.
Cela permettait de verifier que la "decompression" + le write fonctionnait
correctement...
En C++, je n'ai pas acces a md5 de fac,on portable (Win32). Une ide'e ?
  
Quoting Mathieu Malaterre <mathieu.malaterre at kitware.com>:
>    J'ai fais des commit, rien de tres spectaculaire, juste j'ai beaucoup
> de mal a lire les sources de gdcm. Est-ce qu'on peut se mettre d'accord
> sur certaines convention d'ecriture:
J'en reve depuis longtemps. Un coding style NOW ! On met tout cela
par ecrit dans gdcm/DEVELOPPER, et on l'adopte en commun ?

> - pas de printf
J'ai beaucoup de mal a imposer cela a JPR, et je passe un temps fou a
nettoyer ce genre de choses (a utiliser ostringstream, puis dbg.Verbose).
Le pb vient du fait que Jean-Pierre n'utilise pas de debugger et fonctionne
a coup de printf pour debugger. Ensuite ce code n'est jamais nettoye'.
Je suis donc entierement d'accord pour interdire les printf et autre
std::cout, mais cela va poser un gros pb a Jean-Pierre qui DEVRA changer
ses habitudes de travail... Ceci dit emacs + ESC-X gdb fonctionne tres
bien, merci.
Note pour JPR: je peux t'apprendre si tu veux. gdb est a mon avis
  un outil indispensable au programmeur.

> - pas de parentheses aux return:
> return true;
D'ac.

> - pas besoin de void quand la fonction ne prend pas de parametre:
D'ac.

> void foo() {
> ...
> }
Pas d'objection. On peut garder le mode VTK en interne au fonctions 
i.e. conserver des choses du genre
   if (bozo)
   {
   ...
   }
   else if ( )
   {
   ...
   }
versus
   if (bozo) {
   ...
   } else if ( ) {
   ...
   }
?????
C'est juste une question d'habitude, mais dans certaines fonctions
spagheti, cela me simplifie la lecture. Ceci dit, c'est juste une
question de gout et je me plie facilement a la majorite'. L'homogeneite'
du code me semble plus importante que mes preferences ;-)

> - Est-ce que "virtual" est repete meme dans les classes heritees
>  (convention d'ecriture car non necessaire).
Ben a vrai dire, je vois pas l'interet de le repeter, mais je peux
faire l'effort.

> - Il n'y a besoin de specifier explicitement 'inline' lorsqu'une methode
> est implementee dans la classe (fichier header).
D'ac.

> - Tolere pour l'instant: sprintf, FILE*
> Mais j'aimerais passer a une approche plus C++ avec les iostream dans l'avenir
Entierement d'accord. Ceci dit, il y a un certain nombre de choses
que je ne sais pas faire a coup de << ou >>, et donc que je n'ai pas su
proposer a JPR. En particulier je ne sais pas lire/ecrire un paquet de bytes
en gerant la conversion des endianries.
Il faudrait que JPR nous dise quelles sont ces operations atomiques 
en matiere de fread et fwrite, pour proposer une fac,on C++ et portable
de faire.
En attendant j'avais essaye' de factoriser les choses (en particulier les fread)
dans des fonctions genre ReadInt16, mais cela a un peu derape'...

> Je ne reproche rien a personne, je me porte meme volontaire pour les faire.
> Donc est-ce que ca vous va ou vous voulez des choses differentes?
Cela me convient parfaitement, et a vrai dire je milite depuis longtemps
en interne pour faire les choses raisonablement. Je crois qu'il est
capital d'arriver a convaincre notre JPR national (in fine, c'est lui
qui passe le plus du temps sur les internals de gdcm, et c'est donc
lui le plus grand producteur de code). Je me porte volontaire pour aider
JPR dans toutes les difficulte's qu'il rencontrera pour changer ses
habitudes !
Courage Jean-Pierre, on peut le faire !

J'essaye pour l'instant de marquer d'un tag doxygen \todo toutes les
remarques et commentaires qui trainent dans le code. Il est frequent
que JPR pense qu'un pb potentiel surgira, et mette un commentaire du 
style :
  //  if parseDir == false, it should be tagged as an error
et passe a une autre feature. Il est aussi frequent que quand je tombe
sur un bug, la ligne precedente contienne ce genre de commentaire, ce
qui montre que JPR avait pense' au pb potentiel...mais sans le regler,
en general faute de temps [IMHO, c'est la plus mauvaise des excuses,
car on perd encore plus de temps a retrouver le bug apres, plutot que
de traiter le pb correctement ou au moins de mettre le bon fusible].
   Je rajoute donc un \todo devant ce genre de ligne et une fois la doc
genere avec doxygen, on constate dans l'entree TODO, que les occurences
de tels pb potentiels sont tres nombreux.
JPR, si tu nous ecoutes...

FINALEMENT:
 * Je propose que l'on ECRIVE un coding style leger mais OBLIGATOIRE.
 * On passe immediatement en feature freeze.
 * On ecrit une test suite decente en C++.
 * On nettoie intensivement le code en se tenant au coding style.
 * On utilise vraiment gdcm/TODO pour noter ce qui doit-etre fait.
   Quand quelqu'un traite une entre'e, il le signale dans le TODO
   pour eviter de ce marcher sur les pieds avec une gueguerre des commits.

Quand je dis ON, c'est NOUS avec JE !

> Sinon plus grave je suis tombe sur ca dans gdcmFile:
>      lgrTotale /= 3;
>   PixelData = new unsigned char[lgrTotale];
Dans ce cas, je mets un \todo FIXME, mais il faudra un jour les traiter !!!!

> Plus serieusement, on peut pas simplifier les var membres de gdcmFile.
> Ex1:
>   gdcmHeader *Header;
>   bool SelfHeader;
> Y'a t'il vraiment besoin du booleen, est-ce qu'on ne peut pas directement
> regarder si Header est different de NULL ?
VENDU.

> Ex2:
> void* PixelData;
> 
> Ex3:
>   size_t lgrTotaleRaw;
>   size_t lgrTotale;
>   int PixelRead;
>   int Parsed;
C'est a mon sens (avec DicomDir*) la partie la plus confuse du code.
Seul JPR connait se code. Je pense qu'il faudrait qu'il nous expose les
pb de fac,on concise, qu'on se decide, et que l'on nettoie...
JPR m'expliquait hier, qu'il existe trois fonctions pour parser les
pixels. Peut-etre y'a t'il des choses vraiment difficiles...

Ah oui, dans le coding style, il faudrait dire les noms de fonctions,
et membres sont en Anglais, avec des majuscules et porteurs de semantique.
Exemple:
  TotalLength et non lgrTotale
  NewFile et non "niou" (cf Test/PrintDocument.cxx)
Note JPR: tu sais comme le provisoire devient definitif...

> Le choix unsigned char vs char semble complement arbitraire. Par defaut sur
> toutes les plateformes (sauf SUN), char est signed donc il y a bien une
> difference. Est-ce qu'on peut donc tout passer en unsigned char ?
Soit, mais peut-on faire un typedef et ensuite utiliser le meme non partout
(genre guint16). Je recommande comme nom celui defini proprement 
par la norme "ISO C99: 7.18 Integer types" et que l'on trouve dans 
/usr/include/stdint.h (sous Un*x). Microsoft a adopte' cette norme (reuh,
reuh, reuh, je tousse tres fort en souriant) et donc a terme il n'y aura
plus de typedef a faire dans gdcmCommon.h...
En l'occurence le nom canonique semble etre uint8_t !

> 3.
> C'est quoi: bool ParsePixelData(); Normalement VC++ ne sait pas gerer
> les fonctions non implementees...
Oui, et swig wrappant tout fonction declaree, il y a de grande chance
que cela pete a l'edition de lien de libgdcmPython...
Il faudrait que JPR prenne l'habitude de mettre le flag GDCM_WRAP_PYTHON
a ON dans cmake. Rmq: cela pete aussi la compil sous VC++...

> De maniere plus general, j'aurais aime un retour d'experience de cmake.
> J'ai compris que pour l'installation python ce n'etait pas au point.
> Mais en ce qui concerne :
> - la compilation
> - interface pour regler les options de compil
> - les tests
> - l'install des lib c++
Bien que mon experience avec cmake soit encore embryonnaire, voila ce
que je peux en dire:
 * cela nous evite de gerer les .dsw a la main: rien que pour cela
   je suis preneur.
 * je n'ai jamais eu a editer un makefile generes sous un*x, ce qui
   prouve que les choses sont propres (alors que sous les autotools,
   il m'a parfois fallu fouiller pour comprendre mon erreur dans un
   Makefile.am). Mais peut-etre que sans Mathieu, j'aurais du le faire...
 * l'interface manuelle me semble moins sympa que des flags en ligne
   de commande passe's a autogen.sh. A chaque fois que je checkout
   gdcm, je suis force' de modifier les memes choses a la main.
   J'ai cru comprendre qu'en copiant le bon fichier on pouvait eviter
   cela mais je ne sais pas faire...
 * ctest me semble une tres bonne chose. Je n'ai pas encore regarde'
   le lien avec Dart. Je devrais. Je ne sais pas encore comment on
   fait des assert pour une test suite. Il faut que je regarde
   Test/ShowDicom pour apprendre.
 * La gestion de SWIG sera sans doute meilleure dans CMAKE 2.0. 
   Ceci dit, python/distutils est aussi tres le'ge' pour le support
   de swig, alors que c'est droit devant avec les autotools.
   Remarque: le fichier gdcmbin/gdcmPython/gdcm_wrap.cxx n'est pas
             nettoye' par un make clean. C'est regretable, mais
             ce n'est sans doute pas une limitation de cmake.
   Je pense que la passage a swig (patche' certes) dans cable, forcera
   KitWare a un meilleur support de Swig. Wait and see.
 * Pour l'instal des libs, cela pose un ch'ti pb avec les pythoneries.
   En particulier il faut adapater les import pour decliner le nom
   de la .so ou de la .dll a charger (sous Un*x il y a un prefix de lib
   et pas sous WIn32). Ceci dit, je pense que cela respecte plus la facon
   unix de faire, et c'est a nous de nous adapter.
 * La partie python est encore tes largement pe'te'e et difficilement
   exploitable. Il faut qu'on y travaille, mais Benoit et moi avons souffert
   sous Win32 en particulier, a cause de collisions de nom entre les dll et
   les .py (que veut dire import gdcm quand gdcm.dll et gdcm.py existent
   alors que sous Un*x cette collisision est evite'e avec libgdcm.so et
   gdcm.py). Plus la dessus plus tard...

Un bemol tout de meme: c'est Mathieu qui a ecrit tout les CMakefile
et je ne suis pas certain que tout seuls les choses aient ete aussi 
simple... (en particulier je n'ai jamais ouvert la doc de CMAKE pour
essayer de rajouter une fonctionalite': une chose est sure, la doc
des autotools n'est pas triviale...).

   Courage Jean-Pierre: il n'y rien de plus dur a bouger que les esprits.
	Frog.